Forum Rule: Always post complete source code & details to reproduce any issue!
Page 2 of 3 FirstFirst 1 2 3 LastLast
Results 26 to 50 of 74

Thread: Minimal Blink fails with void yield()

  1. #26
    Senior Member+ Frank B's Avatar
    Join Date
    Apr 2014
    Location
    Germany NRW
    Posts
    4,438
    Quote Originally Posted by defragster View Post
    I thought the same Frank - Maybe I made a bad start of it - but I tried it once and it wasn't faster? And yes, lots of overhead to allocate a bool, set the bool, test the bool: a few times over.

    It is nice to just kill the 'weak' yield() - but that is broken. As noted "EventResponder is a WIP it seems PJRC might incorporate best answer" ... but alas a week after posting and no answer . . .

    Manitou first noted in p#3 that it was EventResponder - but referred to IDE build - but this is a bug in TeensyDuino build when the hack to yield() was introduced?

    It was 3 months back - I just posted a comment on github: This yield.cpp change for EventResponder::runFromYield(); breaks removal of 'weak' yield and results in a non-functional upload when that common practice is done. and linked back to this thread.
    Testing one bool is for sure way faster than 7 times calling a function "SerialX.available()"..
    In addition even the tests in the brackets if (!running && flag) {if SerialX.available()...} can be saved if the flag is used in a smart way: Bitwise. Bit #0 for Serial1, bit#1 for Serial2... and setting back to 0 in read()...


    Edit: Maybe something like this:
    Code:
    void yield(void)
    {
        static uint8_t running=0;
    
        if (running) return; // TODO: does this need to be atomic? <- no.
        running = 1;
    int flag = RealNameOfTheVolatileFlag;
    if (flag) {
        if (flag & 0x01) serialEvent();
        if (flag & 0x02) serialEvent1();
        if (flag & 0x04) serialEvent2();
        if (flag & 0x08) serialEvent3();
    #ifdef HAS_KINETISK_UART3
        if (flag & 0x10) serialEvent4();
    #endif
    #ifdef HAS_KINETISK_UART4
        if (flag & 0x20) serialEvent5();
    #endif
    #if defined(HAS_KINETISK_UART5) || defined (HAS_KINETISK_LPUART0)
        if (flag & 0x40) serialEvent6();
    #endif
    }
        running = 0;
    
        EventResponder::runFromYield();
    };
    (should'nt be running = 0 after EventResponder::runFromYield();, or is this intended ?)
    EventResponder::runFromYield(); needs a similar bit in the flag - and its own timer.

    Edit: Doesn't this prevent including of the SerialX Code too ?
    Last edited by Frank B; 09-23-2017 at 07:45 PM.

  2. #27
    Senior Member+ KurtE's Avatar
    Join Date
    Jan 2014
    Posts
    3,503
    There was another discussion about this maybe a month or two ago.

    At the time I proposed doing something similar... But at the time it sounded like maybe should wait until we could implement it with the event objects.

    Some of the stuff at the time, I thought, would set a bit when for example Serial1 received a character and as you mentioned the code really simply first check if FLAG== 0 return...

    I also that that maybe Serial1, would only set this flag if there was if there was a real user implementation for SerialEvent1. The default implementation would simply set/clear flag to tell the Serial1 ISR not to bother setting the flag anymore...


    Some complications that I saw included, when can you clear this flag? You maybe can not clear it when you call serialEvent1 as they may choose not to process all of the outstanding input characters and may choose to defer to process some/all of it on another call... Could maybe be cleared in serial1.read()?

  3. #28
    Senior Member+ Frank B's Avatar
    Join Date
    Apr 2014
    Location
    Germany NRW
    Posts
    4,438
    Quote Originally Posted by Frank B View Post
    Bit #0 for Serial1, bit#1 for Serial2... and setting back to 0 in read()...
    Quote Originally Posted by KurtE View Post
    should wait until we could implement it with the event objects.
    What's the benefit ?

    It moves the problem from yield to systick.
    Last edited by Frank B; 09-23-2017 at 09:14 PM.

  4. #29
    Senior Member+ defragster's Avatar
    Join Date
    Feb 2015
    Posts
    6,378
    Seems the Arduino serialEvent() compatibility the only main() runtime compromise to the 'bare metal'?

    If not used it should just go away - like void yield(void) did before. If a callback Event can be user registered that could be nice.

    As noted Frank - my simple test must have been misapplied - somehow it was not any faster as I did it to check a BOOL.

    Maybe a 'weak' set of serialEvent()'s that set the flag to ZERO. They are set to 1 at compile time. If the user provides serialEvent() that won't set to ZERO and then it will be called in yield? If not replaced it will only be called once?

    <edit>: glanced at code and see this won't work as suggested - until Serial#.available - no call to serialEvent#() is made - this would only help on active ports where serialEvent was not used, unless a second flag were set on any receive - which would require maintaining and checking two flag bits to prevent calling serialEvent#()

    Code:
    void serialEvent(void) __attribute__ ((weak));
    void serialEvent( flag&=!(0x01) );
    void serialEvent1(void) __attribute__ ((weak));
    void serialEvent1( flag&=!(0x02) );
    void serialEvent2(void) __attribute__ ((weak));
    void serialEvent2( flag&=!(0x04) );
    // ...
    Last edited by defragster; 09-24-2017 at 08:44 AM.

  5. #30
    Senior Member+ KurtE's Avatar
    Join Date
    Jan 2014
    Posts
    3,503
    Hi Frank, was simply trying to remember the conversation from a month or two ago. At the time I was trying to make it such that there would be very little performance gain by providing your own yield. My approach at the time was probably almost identical to what you are proposing.

    As for Benefit: The idea I believe is to come up with a consistent solution....

    Also remember saying event Objects does not necessarily imply Systick. Systick is only I believe used for the MillisTimer code.

    There are options to run immediate. Likewise to be called by low priority interrupt, called by yield, ....

    Like you said set back to zero in read (when queue is empty). Likewise if the user calls clear()...

  6. #31
    Senior Member+ Frank B's Avatar
    Join Date
    Apr 2014
    Location
    Germany NRW
    Posts
    4,438
    Ok
    I think I made my suggestions. I also think you'll find a good solution. I just hope I can argue in the future that Teensyduino is as good as bare-metal. It would be a shame if it wasn't. I always thought we agreed that no OS should be created. With a systick with high int- priority doing more than minmum, or that yield(), - with tasks that you can't get rid of easyly - times are changing...
    That is just my opinion on this, and I do not think I need to go into it any further. And I don't want to waste our time on this either.

    EOD (End of discussion for me)

    Have fun,
    Frank
    Last edited by Frank B; 09-23-2017 at 09:59 PM.

  7. #32
    Senior Member duff's Avatar
    Join Date
    Jan 2013
    Location
    Las Vegas
    Posts
    902
    This just bit me with my Zilch library. I too think that event responder stuff needs to be optional like any other library and leave the core as bare metal as possible.

  8. #33
    Senior Member+ defragster's Avatar
    Join Date
    Feb 2015
    Posts
    6,378
    Does my post above in conjunction with Frank's tests make it work to have yield() effect on serialEvent check's reduced? Could be extended with serial buffering code mods even when used - but would minimize the test effort in the bare metal case.

    With OP : I just discovered yield() effect from Events breaks old behavior - but had otherwise figured PJRC would do it right in the end.

    To the same end when the sketch calls the first Event.begin {or .attach /whatever} it could set a 'flag' bit to 1 to call the EventResponder::runFromYield(); in similar fashion - that doesn't break taking out the 'weak' yield().

    I just posted this on the TD 1.39 released thread:
    Quote Originally Posted by defragster View Post
    EventResponder::runFromYield(); in yield breaks standing use of replacing/removing the 'weak' yield() to keep Teensy Bare Metal.

    Minimal-Blink-fails-with-void-yield()

  9. #34
    Senior Member+ KurtE's Avatar
    Join Date
    Jan 2014
    Posts
    3,503
    Quote Originally Posted by Frank B View Post
    Ok
    I think I made my suggestions. I also think you'll find a good solution. I just hope I can argue in the future that Teensyduino is as good as bare-metal. It would be a shame if it wasn't. I always thought we agreed that no OS should be created. With a systick with high int- priority doing more than minmum, or that yield(), - with tasks that you can't get rid of easyly - times are changing...
    That is just my opinion on this, and I do not think I need to go into it any further. And I don't want to waste our time on this either.
    I think we are on the same page! I had a partial implementation back then doing more or less exactly what you suggested. I did have an extra test at the start, that tested the whole flag to zero and returned immediate... Figured that would probably be the majority case, especially with my hacks that if default SerialEvent functions were called then it never again sets that bit ... I don't think I have that code sitting around any more...

    Maybe one of us should try it again and see if Paul will accept a Pull Request.

    As for your other Pull request as I mentioned I wondered if there would be an issue with TeensyThreads...

    So my thoughts before you did it was to move the systick timer back to the source file it originally was in and put in call that is conditional. Something like:
    Code:
    void   (*run_from_timer_func)() = nullptr;
    void setRunFromTimerFunc(void (*f)()) {run_from_timer_func = f;}
    
    extern "C" volatile uint32_t systick_millis_count;
    
    void systick_isr(void)
    {
    	systick_millis_count++;
            if (run_from_timeer_func) {
                (*run_from_timer_func)();
            }
    }
    Then have the MillisTimer code code do a call with it's ( MillisTimer::runFromTimer()) function on the first call to begin (or beginRepeating)...

    So in the default case we have a slight overhead to test for NULL and jump...

  10. #35
    Senior Member PaulStoffregen's Avatar
    Join Date
    Nov 2012
    Posts
    17,482
    I'm looking at this today.

    Sorry about the delayed reaction. This thread started last week while Robin was out of town. Normally she does most of the day-to-day operational stuff to run PJRC, so I can focus on tech questions and (sometimes) software dev. When she's out to town, I fall pretty far behind on tech issues. Usually I prioritize answering custom support questions first. Software bugs and actually developing anything new pretty much grinds to a halt. I know it's frustrating, to discuss a serious bug and not get any reply from me. Please try to understand PJRC is only 4 people, 2 of whom test and package products, so when we're down 1 person it really shows. I try to always keep up with the forum, but sometimes it's just not practical.

  11. #36
    Senior Member+ defragster's Avatar
    Join Date
    Feb 2015
    Posts
    6,378
    Good to see you Paul - hopefully our neurotic posting helps . . .

    Won't the EventResponder have some initial wake (.attach) up code called in or before setup()? It could one time replace the pointer to systick_isr(void) with systick_isr_ER(void)? Where the _ER function is without conditional:
    Code:
    void systick_isr_ER(void)
    {
    	systick_millis_count++;
            (*run_from_timer_func)();
    }
    Of course if that vector is not == systick_isr() - then it is a chaining rabbit hole for cases that already redefine that : Zilch or TeensyThreads . . .

  12. #37
    Senior Member duff's Avatar
    Join Date
    Jan 2013
    Location
    Las Vegas
    Posts
    902
    EDIT: It does work, disregard below.



    I updated the core (1.39 / T3.6 / 180MHz / Faster / Serial) with Franks patches here and tested it with the sketch below but overriding yield function still crashes the system, no usb no blink:

    Code:
    int led = 13;
    
    void setup() {
      // initialize the digital pin as an output.
      pinMode(led, OUTPUT);
      digitalWriteFast(led, HIGH);   // turn the LED on (HIGH is the voltage level)
    }
    
    void loop() {
      digitalWriteFast(led, HIGH);   // turn the LED on (HIGH is the voltage level)
      delay(1000);               // wait for a second
      digitalWriteFast(led, LOW);    // turn the LED off by making the voltage LOW
      delay(1000);               // wait for a second
    }
    
    void yield(void) {
    
    };
    Last edited by duff; 09-24-2017 at 12:18 AM.

  13. #38
    Senior Member+ defragster's Avatar
    Join Date
    Feb 2015
    Posts
    6,378
    Quote Originally Posted by duff View Post
    I updated the core (1.39 / T3.6 / 180MHz / Faster / Serial) with Franks patches here and tested it with the sketch below but overriding yield function still crashes the system, no usb no blink:
    @duff: Somehow it worked and blinked for me when I hand made those changes (T_3.1 w/NO USB): Minimal-Blink-fails-with-void-yield() - though I have reverted them now.

  14. #39
    Senior Member PaulStoffregen's Avatar
    Join Date
    Nov 2012
    Posts
    17,482
    I'm running Defragster's code here on a Teensy 3.1 and it does indeed reproduce the problem.

  15. #40
    Senior Member+ Frank B's Avatar
    Join Date
    Apr 2014
    Location
    Germany NRW
    Posts
    4,438
    Duff:Might be the caching bug in the builder?

  16. #41
    Senior Member PaulStoffregen's Avatar
    Join Date
    Nov 2012
    Posts
    17,482
    Quote Originally Posted by Frank B View Post
    Duff:Might be the caching bug in the builder?
    Unlikely. I just checked with Arduino 1.8.1. Exactly the same problem.

    Much as I'd like it to be toolchain, it seems this bug is entirely my fault.

  17. #42
    Senior Member duff's Avatar
    Join Date
    Jan 2013
    Location
    Las Vegas
    Posts
    902
    So it looks like on my computer the Teensydunio core gets cached and any edits after the first compile to the core are not picked up, so frank your patch does work now! Not sure why the core is cached on my system, macOS Sierra but that another issue for me.

  18. #43
    Senior Member+ defragster's Avatar
    Join Date
    Feb 2015
    Posts
    6,378
    Glad you re-produced the issue Paul.

    re #41: I think Frank #40 was saying to duff - his attempt to use Frank's solution failed to fully rebuild after he made the edits and that resulted in his post #37 saying it didn't work.

    <edit> : confirmed in Cross Post - builder didn't catch changed files for core rebuild.

  19. #44
    Senior Member duff's Avatar
    Join Date
    Jan 2013
    Location
    Las Vegas
    Posts
    902
    I can say for sure the teensyduino 1.38 beta2 does not have the yield problem.

  20. #45
    Senior Member+ defragster's Avatar
    Join Date
    Feb 2015
    Posts
    6,378
    Quote Originally Posted by duff View Post
    I can say for sure the teensyduino 1.38 beta2 does not have the yield problem.
    That makes sense:
    Teensyduino 1.38 Beta #3
    Started by*Paul,*08-22-2017*02:43 PM
    - - - - - - - -
    Changes since Teensyduino 1.38-beta2:

    ...
    [Experimental] EventResponder

  21. #46

  22. #47
    Senior Member+ defragster's Avatar
    Join Date
    Feb 2015
    Posts
    6,378
    It Works.and Blinks.

    On my IDE 1.8.3 I reinstalled TD 1.39.
    edited 'weak' line ~#148 in mk20dx128.c to :: void systick_isr(void);

    Smallest w/LTO and NO SERIAL::
    BlinkMin with :: // void yield(void) {};
    BlinkMin with :: void yield(void) {};

    It grew 200 bytes from 1712 bytes ???
    Sketch uses 1920 bytes (0%)
    Global variables use 492 bytes (0%)
    Also worked Smallest w/LTO and SERIAL
    Sketch uses 4564 bytes (1%)
    Global variables use 2036 bytes (3%)

  23. #48
    Senior Member duff's Avatar
    Join Date
    Jan 2013
    Location
    Las Vegas
    Posts
    902
    Quote Originally Posted by PaulStoffregen View Post
    Works with Zilch too

  24. #49
    Senior Member PaulStoffregen's Avatar
    Join Date
    Nov 2012
    Posts
    17,482
    Yeah, it increases code size slightly. Part is essential code that was getting incorrectly optimized away. The rest is the static parts of MillisTimer and EventResponder.

    I know right now the value of these experimental features may seem questionable. But I'm confident in the long term a couple hundred bytes a code space and a dozen bytes of RAM will seem like a very small price to pay for allowing many libraries to interoperate easily.

  25. #50
    Senior Member+ defragster's Avatar
    Join Date
    Feb 2015
    Posts
    6,378
    Death, Taxes and code bloat

    No doubt when the Events code is tuned and working it will be for the good.

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •