Skip to content

Allow override for SoftwareSerial timer ISR priority #751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sjasonsmith opened this issue Nov 2, 2019 · 13 comments · Fixed by #755
Closed

Allow override for SoftwareSerial timer ISR priority #751

sjasonsmith opened this issue Nov 2, 2019 · 13 comments · Fixed by #755
Labels
enhancement New feature or request

Comments

@sjasonsmith
Copy link
Contributor

Is your feature request/improvement related to a problem? Please describe.

The recently added SoftwareSerial library uses the default timer priority for software bit timing. For many processors, this defaults to preemption level 14, which will cause bit timing issues when integrating with systems using other interrupts on the system.

This became apparent when integrating SoftwareSerial with Marlin when testing the following Marlin PR:
MarlinFirmware/Marlin#15655

Describe the solution you'd like

I suggest the following changes to the SoftwareSerial library. I am willing to contribute these changes, but wanted input on whether these changes were likely to be accepted before creating a PR.

  1. Replace TIMER_SERIAL constant with SWSERIAL_TIMER_NUM.

    • Prefix clearly communicates that this constant impacts SoftwareSerial.
    • Providing timer number instead of name simplifies construction of interrupt name at build time.
      • This change could be skipped if there is a good way to determine an interrupt number from a timer base address at runtime (Since TIMx is actually a macro to the base address).
    • #error can be used to communicate change if TIMER_SERIAL is defined
  2. Add SWSERIAL_TIMER_INT_PRIO to override interrupt priority

    • Only change the priority if the constant is defined, else use default.
    • Alternatively, default to highest interrupt priority for best bit-timing performance, and allow users to downgrade priority with constant.
  3. Expose additional macros with SWSERIAL prefix, allowing users to customize behavior if needed:

    • SWSERIAL_FORCE_BAUD_RATE (Currently override-able as FORCE_BAUD_RATE)
    • SWSERIAL_OVERSAMPLE
    • SWSERIAL_HALFDUPLEX_DELAY

Describe alternatives you've considered
Working around this in Marlin was quite cumbersome. We were able to get this to communicate by arranging other timer interrupts around the default timer interrupt level, but that solution is not ideal. It likely causes other timing issues due to changes to prioritization among hardware ISRs, etc.

I attempted to set the timer ISR priority from within Marlin, but the change did not persist. I believe that when the timer is reset during a baud rate switch (setSpeed) the timer is being restored to its default priority level.

@LinoBarreca
Copy link
Contributor

LinoBarreca commented Nov 2, 2019

...or instead of defining them as constants (at build time) provide some methods to change the values on the instantiated object which reflects what the user wants to do. If it's simpler for you (because you don't change them once instantiated) you could provide some alternative constructors
for example SoftwareSerial(timer, priority)
the oversample and halfduplex could be just properties that the user can set to change the object behaviour.

@fpistm
Copy link
Member

fpistm commented Nov 2, 2019

I guess @LinoBarreca proposed the best option.

@sjasonsmith
Copy link
Contributor Author

...or instead of defining them as constants (at build time) provide some methods to change the values on the instantiated object which reflects what the user wants to do

Thanks for the suggestion. I considered this, and it would be my preferred option if this were brand-new code rather than an existing interfaces.

As it is now, I see the following problems with adding run-time options to the existing interface:

  1. The same SoftwareSerial interfaces exists on multiple platforms. Adding capabilities to the interface would require applying the same changes for AVR, 176x, etc. If capabilities were added only for one platform, generic code would be unlikely to use them.

  2. Runtime alternatives would not be accessible if SoftwareSerial is used indirectly through other libraries, unless those libraries in-turn exposed them to users. Marlin using SoftwareSerial indirectly through TMCStepper is an example. The #define option allows behavior to be altered without requiring changes to library code.

@fpistm
Copy link
Member

fpistm commented Nov 2, 2019

That make sense. Do not hesitate to provide a PR

@sjasonsmith
Copy link
Contributor Author

Of the changes I proposed, the interrupt priority is the most important aspect.

I could add a static function like SoftwareSerial::setTimerIntPriority to change the priority at runtime. That might be easier to use from Marlin anyway.

The only issue I have implementing that is I haven't yet found a simple way to get an interrupt number from a timer name. Changing the #define to accept a timer number instead of a name allows it to be easily derived at build-time using macros.

I'll play with this today. If I come up with something I like and haven't heard more suggestions, I'll put up a PR and it can be ripped apart there 😃

@sjasonsmith
Copy link
Contributor Author

I could add a static function like SoftwareSerial::setTimerIntPriority to change the priority at runtime. That might be easier to use from Marlin anyway.

This is contrary to my previous comment about not wanting to change the interface. In this case it seems like less of an issue, since interrupt priority is already platform-specific, and would be configured by board-specific code rather than generic library code.

@LinoBarreca
Copy link
Contributor

you are right about "existing interfaces".
but you are keeping the interface as is, actually.
You keep what you have (compatibility with old code) and add new, platform specific, things.
You can't keep compatibility anyway (unless you keep it so generic that you can't unleash the features of more modern platforms) because platforms are necessarily different....
Adding something isn't breaking the interface. you still implement the interface methods. and add your own.

@LinoBarreca
Copy link
Contributor

..but we agree on one thing: as is, the softwareserial, has a big problem in real life usage scenarios :D

@sjasonsmith
Copy link
Contributor Author

..but we agree on one thing: as is, the softwareserial, has a big problem in real life usage scenarios :D

We probably agree on lots of things, but sometimes communication breaks down across text, languages, and 8 time zones!

@fpistm
Copy link
Member

fpistm commented Nov 5, 2019

@sjasonsmith , @LinoBarreca
FYI (as it seems you use SoftwareSerial for HalfDuplex), the core now support HalfDuplex also with HardwareSerial thanks to @ghent360 . This could be fine to use it to free MCU ressources if you can use an U(S)ART Tx pin.
https://github.com/stm32duino/wiki/wiki/API#enable-half-duplex-mode

@sjasonsmith
Copy link
Contributor Author

Thanks @fpistm, that is good information. Unfortunately we are working with boards that are already built, and often do not have hardware UARTs wired to the right places. I would always choose HardwareSerial if I could.

Do you know approximately when the next release will occur, which could include the Timer/SoftwareSerial fixes in #749 and #755?

@fpistm
Copy link
Member

fpistm commented Nov 5, 2019

This should be in december.
I try to made a release each 3 month (1/quarter).

@LinoBarreca
Copy link
Contributor

@fpistm we use a mix, actually, of soft/hard serials, full/half duplex according to the board variant (how the manufacturer actually wired the chip) and the user configuration. Good to know that, for those who have the option to use a HardwareSerial, we already have the fix if the TX/RX is the same. well done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants