Problem with volatile array of struct in Arduino 1.6.12

I have an Arduino program to control X-10 using Thomas Mittet's X-10 library: github.com/tmittet/x10/

This works using the Arduino 1.6.4 IDE but I'd like to update to 1.6.12. It doesn't want to play.

My c++ is pretty, well, sketchy but I've narrowed the problem down to a section in the x10ex.h include file where the library declares a volatile array of struct items, which is used to hold the message buffer and persist it between calls to the interrupt service routine.

I sort of understand what the volatile keyword is there for -

Essentially, the compiler will assume the Arduino is able to hold a value unchanged in a register for at least a reasonable period of time. It uses this assumption to streamline the code, working with copies of values in a register as a sort of cache rather than reloading from main memory all the time. If an interrupt happens, though, and the routine that services the interrupt makes a change to that value in the main memory then that assumption doesn’t hold. The volatile keyword tells the compiler to assume that any copy of this value that happens to be in a register is suspect and has to be re-read from main memory any time it’s needed.

Since the X-10 transmit and receive code is entirely interrupt-driven it’s pretty easy to see why volatile variables would make sense.

I've stripped everything down to about the smallest chunk of code that still demonstrates the problem - so don't pull me up on the fact that the volatile keyword is completely pointless in the code below! I really just need to understand what is wrong with this so I can apply it to the real use case.

/************************************************************************/
/*                                                                      */
/* Code to demonstrate problem with in Arduino IDE 1.6.12               */
/* when using volatile keyword as defined in                            */
/*                                                                      */
/* X10 PLC library test sketch with Serial support, v1.6.               */
/*                                                                      */
/* Written by Thomas Mittet (code@lookout.no) June 2012.                */
/************************************************************************/
#define X10_BUFFER_SIZE      17

// Used when buffering messages
struct X10msg 
{
  uint32_t message;
  uint8_t repetitions;
};

class X10ex
{
  public:
    X10ex();
    // Public methods
    void begin();

  private:

/************************************************************************/
/*                                                                      */
/* The Problem:                                                         */
/*                                                                      */
/************************************************************************/

    X10msg volatile sendBf[X10_BUFFER_SIZE];

//    X10msg sendBf[X10_BUFFER_SIZE];
};

X10ex::X10ex()
{}

//////////////////////////////
/// Public
//////////////////////////////

void X10ex::begin()
{}

X10ex x10ex = X10ex();

void setup() {}

void loop() {
  // put your main code here, to run repeatedly:

}

Using the (commented) non-volatile declaration instead of the volatile one it compiles fine. In the real use-case of course I get problems which I'm attributing to this value changing during the interrupt service routine. Using the volatile version I get the error report:

Test_volatile:41: error: use of deleted function 'X10ex::X10ex(X10ex&&)'

 X10ex x10ex = X10ex();

                     ^

C:\Users\Phil\Documents\Arduino\Test_volatile\Test_volatile.ino:19:7: note: 'X10ex::X10ex(X10ex&&)' is implicitly deleted because the default definition would be ill-formed:

 class X10ex

       ^

Test_volatile:19: error: no matching function for call to 'X10msg::X10msg(volatile X10msg)'

C:\Users\Phil\Documents\Arduino\Test_volatile\Test_volatile.ino:19:7: note: candidates are:

C:\Users\Phil\Documents\Arduino\Test_volatile\Test_volatile.ino:13:8: note: X10msg::X10msg()

 struct X10msg 

        ^

C:\Users\Phil\Documents\Arduino\Test_volatile\Test_volatile.ino:13:8: note:   candidate expects 0 arguments, 1 provided

C:\Users\Phil\Documents\Arduino\Test_volatile\Test_volatile.ino:13:8: note: constexpr X10msg::X10msg(const X10msg&)

C:\Users\Phil\Documents\Arduino\Test_volatile\Test_volatile.ino:13:8: note:   no known conversion for argument 1 from 'volatile X10msg' to 'const X10msg&'

C:\Users\Phil\Documents\Arduino\Test_volatile\Test_volatile.ino:13:8: note: constexpr X10msg::X10msg(X10msg&&)

C:\Users\Phil\Documents\Arduino\Test_volatile\Test_volatile.ino:13:8: note:   no known conversion for argument 1 from 'volatile X10msg' to 'X10msg&&'

exit status 1
use of deleted function 'X10ex::X10ex(X10ex&&)'

The original author seems to have gone quiet on his blog but a couple of other people seem to have hit the same problem.

So - can anyone tell me what's changed between 1.6.4 and 1.6.12 to exhibit this problem? Can anyone supply the correct workaround?

If you want more detail on what I'm up to I have a blog article on this you can check out - but it really shouldn't be necessary to understand the problem. If you're interested: hypercene.wordpress.com/2016/11/02/x10-ding-mysensors/

I don't believe that I have ever seen the volatile keyword used after the type. Maybe move the volatile keyword before the type.

First thing I tried ;-)

Sorry - didn't post all the assorted junk tests I did, but that one was pretty early on. I've just tested it again and cross-checked the error listing with the one above - it's identical.

Thanks for the response, though.

PaulS: I don't believe that I have ever seen the volatile keyword used after the type. Maybe move the volatile keyword before the type.

I have, and it makes no difference. Having it after actually makes more sense based on the right-to-left qualifier rule for type modifications. For example, do you know what the difference is between const int*, int const *, and int * const? You can replace const with volatile in all those type and have the same puzzle.

As for hypercene's problem, change X10ex x10ex = X10ex(); to X10ex x10ex();

It also works removing volatile from the sendBf declaration. You can remove it and declare the x10ex variable volatile instead.

I think what's happening is that the volatile member variable is limiting the optimizations the compiler can make. Without the volatile member, the compiler can optimize that line into a default construction (which is what my fix is). With the volatile member, it has to be done as default constructing a temp variable, then moving it into the x10ex variable. For some reason, the compiler is not able to make a default move constructor, which is why you're getting this error: "'X10ex::X10ex(X10ex&&)' is implicitly deleted"

Move semantics are a new thing in C++ 11, and I haven't used them much myself.

OK - that sounds similar to some of the things I was reading and not understanding enough to act on.

I've tried out the change from X10ex x10ex = X10ex(); to X10ex x10ex();

You're quite right - it now compiles in my test program. I'll go away and try to understand it properly and apply it to the actual use case. I suspect I'll be back wearing a confused expression....

Thanks!

I've now gone back to the original code - and of course it's not quite as simple as I made it look. However - expanding the logic does seem to solve the problem. The original variable declaration was actually:

X10ex x10ex = X10ex(
  2, // Zero Cross Interrupt Number (2 = "Custom" Pin Change Interrupt)
  4, // Zero Cross Interrupt Pin (Pin 4-7 can be used with interrupt 2)
  5, // Power Line Transmit Pin 
  6, // Power Line Receive Pin
  true, // Enable this to see echo of what is transmitted on the power line
  powerLineEvent, // Event triggered when power line message is received
  1, // Number of phases (1 = No Phase Repeat/Coupling)
  50 // The power line AC frequency (e.g. 50Hz in Europe, 60Hz in North America)
);

Changing this to

X10ex x10ex(
  2, // Zero Cross Interrupt Number (2 = "Custom" Pin Change Interrupt)
  4, // Zero Cross Interrupt Pin (Pin 4-7 can be used with interrupt 2)
  5, // Power Line Transmit Pin 
  6, // Power Line Receive Pin
  true, // Enable this to see echo of what is transmitted on the power line
  powerLineEvent, // Event triggered when power line message is received
  1, // Number of phases (1 = No Phase Repeat/Coupling)
  50 // The power line AC frequency (e.g. 50Hz in Europe, 60Hz in North America)
);

resolves the compiler errors - so first and foremost, thanks: that's my problem solved!

As far as I understand, this isn't actually a default constructor because it has a parameter list - so the problem must be something in the difference between those two styles.

Now, I don't quite get the bit that's probably real obvious to more savvy coders out there. What's the actual difference between

MyClass objMyThing = MyClass (param1, param2 ); and MyClass objMyThing (param1, param2 );

and why would one use each of the two? Just roughly, of course: I'm not looking to waste anyone's time!

The assignment operator.

Helpful. Thanks.

hypercene: MyClass objMyThing = MyClass (param1, param2 ); and MyClass objMyThing (param1, param2 );

and why would one use each of the two? Just roughly, of course: I'm not looking to waste anyone's time!

In most cases, the compiler is allowed to get rid of the assignment operator as part of its optimizations, and will refactor the first to be equivalent to the second.

With volatile-qualified variables in the mix, optimizations are much more limited and the compiler is not allowed to do that particular refactoring. It has to do a move assignment from a temporarily constructed object.

So this really is about how the compiler goes about interpreting the code rather than what the code actually does?

I can live with that: c's been doing my head in off and on for the best part of 30 years and I don't expect it to stop any time soon.

C++

Ah well, it was c I started with back in the 80's when it began doing my head in, see?

All c++ really achieved from that point of view was that it started objectively doing my head in.

hypercene: So this really is about how the compiler goes about interpreting the code rather than what the code actually does?

Yes, it's about different optimizations the compiler can and can't do depending on which variables are declared volatile.

You might find it easier removing volatile from the member variable declaration and declaring the whole structure variable volatile: X10ex volatile x10ex( /* whatever */ );