Adds local loopback to TractionThrottle. #756
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR improves behavior when there is more than one TractionThrottle in the same openlcb::node_. Specifically, when these are assigned to the same dst_ locomotive, the Train Node will never send back echoes of the packets. This means that the simultaneous updates are missing. This PR makes all TractionThrottle objects lined up in a linked list upon creation. When a locomotive control commands (speed, estop, fn) is sent out, a local loopback will walk the linked list and identify if there are any other tractionthrottle objects assigned to the same loco. If so, the message will be handed over to it for update callbacks.
…r adding the second train node.
Ensures that the listener link doesn't get prematurely removed. When more than one throttle is listening to the same train node, any throttle would remove the listener link, leaving the others without a registration. Now only the last throttle remaining will remove the link.
bakerstu
approved these changes
Dec 23, 2023
| { | ||
| { | ||
| // finds next instance that's interesting | ||
| AtomicHolder h(LinkedObject<TractionThrottle>::head_mu()); |
There was a problem hiding this comment.
I'm not crazy about holding an atomic section for the loop through of an unbounded list. Is there an existing mutex we could use instead? What about a task scheduler lock instead?
There was a problem hiding this comment.
Under linux an Atomic is actually a mutex.
Under freertos it is a critical section, which means that (on an M3/M4) only priority zero interrupts can go through.
So the important question is how many iterations are there here and how long do they take.
- For almost all devices there are at most two iterations. Throttles typically have one or two such objects.
- For command stations there can be up to 64 such objects. (63 for NCE, 31 for XNet). Definitely not unbounded, because each tractionthrottle is about 100 bytes or more.
One iteration is just a pointer chasing and comparing some memory bytes. I expect it to be say 0.5 to 1 usec (few dozen instructions).
As a result, I expect a non-p0 interrupt jitter of about 0.05 to 0.1 millisecond to be caused by this.
Non-P0 interrutps are things like CAN-bus (1 msec per interrupt) or UART (where we have a FIFO , unless in NCE, where the interrupt is one per msec as well).
It's not super practical to replace this with a mutex or a scheduler lock:
- LinkedObject<> is used elsewhere, if we want to replace the locking, we need to copy the code or impact every other use as well (examples are Executor<> and StateFlowWithQueue<>)
- scheduler lock doesn't exist in linux. We don't have an abstraction for it. It's also unclear what it would do in a multi-core ESP32.
- Another alternative might be to jump to the main executor for sending outgoing packets, but this is also not exciting, because it would probably break all unit tests. It also doesn't guarantee that the links don't change if an object gets constructed or deleted on another thread.
balazsracz
added a commit
that referenced
this pull request
Feb 2, 2024
* master: Fixes file comment. Adds an application (hub_test) for testing the throughput of a hub or a CAN-bus (#762) Adds a helper function to decode a 14-bit 9.2.1.1 address into address type and raw address. (#766) Fix broken test. Accessory packet refactoring and POM support (#764) Adds support for Offset(n) attribute in the CDI (#765) Adds local loopback to TractionThrottle. (#756) Fix test flakiness. Fix compile error on FdUtils under freertos.
balazsracz
added a commit
that referenced
this pull request
Feb 4, 2024
* master: Upintegrate changes from the OpenMRNIDF repository (#771) Adds support for DCC extended accessories (#769) Fix incorrect consumer identified message being emitted by dcc accy producer. (#768) Avoids rendering hidden segments. (#767) Adds trailing zero to the cdi XML file written to the filesystem. (#777) Fix target subdirectory name (#775) Fixes file comment. Adds an application (hub_test) for testing the throughput of a hub or a CAN-bus (#762) Adds a helper function to decode a 14-bit 9.2.1.1 address into address type and raw address. (#766) Fix broken test. Accessory packet refactoring and POM support (#764) Adds support for Offset(n) attribute in the CDI (#765) Adds local loopback to TractionThrottle. (#756) Fix test flakiness. Fix compile error on FdUtils under freertos.
balazsracz
added a commit
that referenced
this pull request
Feb 4, 2024
* bracz-idf-downintegrate: Upintegrate changes from the OpenMRNIDF repository (#771) Fix comments. Adds support for DCC extended accessories (#769) Fix incorrect consumer identified message being emitted by dcc accy producer. (#768) Avoids rendering hidden segments. (#767) Adds trailing zero to the cdi XML file written to the filesystem. (#777) Fix target subdirectory name (#775) Fixes file comment. Adds an application (hub_test) for testing the throughput of a hub or a CAN-bus (#762) Adds a helper function to decode a 14-bit 9.2.1.1 address into address type and raw address. (#766) Fix broken test. Accessory packet refactoring and POM support (#764) Adds support for Offset(n) attribute in the CDI (#765) Adds local loopback to TractionThrottle. (#756) Fix test flakiness. Fix compile error on FdUtils under freertos.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR improves behavior when there is more than one TractionThrottle in the same openlcb::node_. Specifically, when these are assigned to the same dst_ locomotive, the Train Node will never send back echoes of the packets. This means that the simultaneous updates are missing.
This PR makes all TractionThrottle objects lined up in a linked list upon creation. When a locomotive control commands (speed, estop, fn) is sent out, a local loopback will walk the linked list and identify if there are any other tractionthrottle objects assigned to the same loco. If so, the message will be handed over to it for update callbacks.