Page 1 of 1

Incorrect return from chSchGoSleepTimeoutS()  Topic is solved

Posted: Tue Jan 10, 2017 6:13 pm
by wgreiman
recently the avr-gcc compiler used by Arduino was change and now chSchGoSleepTimeoutS() returns the wrong status for an old version of nil.

I found the problem and checked for a fix in the latest version of nil. It appears it would have the same problem.

At line 680 of ch.c, trunk 10022, the wrong value is returned.

Code: Select all

    if (NIL_THD_IS_READY(ntp)) {
      nil.current = nil.next = ntp;
      if (ntp == &nil.threads[CH_CFG_NUM_THREADS]) {
        CH_CFG_IDLE_ENTER_HOOK();
      }
      port_switch(ntp, otp);
      return nil.current->u1.msg;  <<------------ Line 680.
    }


If I replace the return with this it is correct.

Code: Select all

 return otp->u1.msg;


Also it works if I make the the "current" member of nil_system_t volatile.

What is the best fix?

Re: Incorrect return from chSchGoSleepTimeoutS()

Posted: Tue Jan 10, 2017 6:27 pm
by Giovanni
Hi,

Thanks for finding, I see problems with both solutions:

Making current volatile would probably increase code size, using "otp" could make the compiler save more registers to pass the value across port_switch() worsening stack usage.

Could you try to add a:

Code: Select all

  asm volatile ( : : : "memory");


To port_switch()? this should make the compiler understand the memory can be modified across the function. In other ports port_switch() is implemented into an asm module so this issue does not occurs.

Giovanni

Re: Incorrect return from chSchGoSleepTimeoutS()

Posted: Tue Jan 10, 2017 6:54 pm
by wgreiman
I tried putting this line in port_switch() with no improvement. I tied at the beginning and just befor the "ret"

Code: Select all

  asm volatile ("" : : : "memory");


If I put it between the port_switch() call and the return, like this, it works. The program actually uses two bytes less flash.

Code: Select all

      port_switch(ntp, otp);
      asm volatile ("" : : : "memory");       
      return nil.current->u1.msg;

Re: Incorrect return from chSchGoSleepTimeoutS()

Posted: Tue Jan 10, 2017 7:32 pm
by Giovanni
I see, however placing it in the portable code is not appropriate.

Probably the proper fix would be to write port_switch() into and asm module like other ports do, the compiler fails to see that the function alters memory for some reason, it assumes it to be pure.

Other things that could be tried:
1) Add ": : : "memory"" to one of the existing statements but I don't think it will change.
2) Add "noinline" attribute to port_switch().
3) Rename port_switch() in _port_switch() then create a macro in chcore.h port_switch() that calls _port_switch() then includes the memory barrier. This way it would not be in the portable code.

Giovanni

Re: Incorrect return from chSchGoSleepTimeoutS()

Posted: Tue Jan 10, 2017 8:01 pm
by wgreiman
Giovanni,

Thanks for all the help. The new avr-gcc compiler is causing me grief in a lot of my open source software.

I tried adding ":::"memory" to an existing statement earlier and it failed.

I didn't try adding "noinline" since I like the macro idea.

I defined this macro in chcore.h and it works.

Code: Select all

#define port_switch(ntp, otp) {_port_switch(ntp, otp); \
                               asm volatile ("" : : : "memory");}

Re: Incorrect return from chSchGoSleepTimeoutS()

Posted: Sat Oct 20, 2018 8:50 pm
by Giovanni
Thanks, I will do the same change in next release.

Giovanni