DCC-EX / CommandStation-EX

EX-CommandStation firmware from DCC-EX. Includes support for WiFi and a standalone WiThrottle server. A complete re-write of the original DCC++.
https://dcc-ex.github.io/
GNU General Public License v3.0
155 stars 107 forks source link

Adjust forget commands to respect the direction of locos #233

Closed nic547 closed 2 years ago

nic547 commented 2 years ago

For context: I'm trying to write my own software that interfaces with DCC++ EX. There I need to deal with the fact that only a limited number of locos can be "active" at a time. I would like to "forget" locos that aren't actually in use to keep the number of active locos down. I don't think there's a good way to figure out which locos are parked on the layout and which ones have been removed from the layout, so my plan was to forget both, as a loco being "unknown" and not getting speed reminders is kinda the default anyway.

Currently the "forget locos" commands send e-stops without the direction bit set, e.g. they're backwards e-stops. When a loco is still on the layout, this can lead to the headlights facing the wrong way, which is a bit unfortunate IMHO.

With this PR, instead of sending the "backwards e-stop" packet to locos the CS will send the e-stop packet with the direction the locos should already have. While I personally only require the change to the "forget this loco" command, I have also adjusted the "forget all locos" command to be consistent. Instead of sending a broadcast e-stop, it now only sends the e-stop to locos in the speed table.

This PR is probably a breaking chance, while not explicitly documented I would assume that other software might rely on the current behaviour (at least mine does). So it's not quite obvious if this change is worth it or if there are better options.

Also thanks to haba on discord for helping me a bit with this.

Asbelos commented 2 years ago

I think calling updateLocoReminder for a loco that isn't in the list will add it to the list.. On 4 May 2022 13:02, nic547 @.***> wrote: For context: I'm trying to write my own software that interfaces with DCC++ EX. There I need to deal with the fact that only a limited number of locos can be "active" at a time. I would like to "forget" locos that aren't actually in use to keep the number of active locos down. I don't think there's a good way to figure out which locos are parked on the layout and which ones have been removed from the layout, so my plan was to forget both, as a loco being "unknown" and not getting speed reminders is kinda the default anyway.

Currently the "forget locos" commands send e-stops without the direction bit set, e.g. they're backwards e-stops. When a loco is still on the layout, this can lead to the headlights facing the wrong way, which is a bit unfortunate IMHO. With this PR, instead of sending the "backwards e-stop" packet to locos the CS will send the e-stop packet with the direction the locos should already have. While I personally only require the change to the "forget this loco" command, I have also adjusted the "forget all locos" command to be consistent. Instead of sending a broadcast e-stop, it now only sends the e-stop to locos in the speed table.

DCC::forgetLoco calls the setThrottle function twice - I'm not sure why, but assumed it's to ensure that the loco actually gets the packet, since it's not gonna get any reminders. DCC::forgetAllLocos doesn't send it twice. Should I adjust one of them?

Using DCC::setThrottle instead of DCC::setThrottle2 results in a call to DCC::updateLocoReminder. If I understand the code correctly, calling the updateLocoReminder with for a loco that isn't in the speedTable should do nothing. Is it ok to rely on that behaviour?

This PR is probably a breaking chance, while not explicitly documented I would assume that other software might rely on the current behaviour (at least mine does). So it's not quite obvious if this change is worth it or if there are better options. Also thanks to haba on discord for helping me a bit with this.

You can view, comment on, or merge this pull request online at:   https://github.com/DCC-EX/CommandStation-EX/pull/233

Commit Summary ef009a0 Adjust forget commands to respect the direction of engines

File Changes (1 file)

M
DCC.cpp
(13)

Patch Links: https://github.com/DCC-EX/CommandStation-EX/pull/233.patchhttps://github.com/DCC-EX/CommandStation-EX/pull/233.diff

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: @.***>

nic547 commented 2 years ago

Ok, I have adjusted that part. I extracted the calculation of the speed byte into it's own function, that I then use in the forget commands, so that I can call setThrottle2 and avoid the call to updateLocoReminder.

Is there an easy way to validate that the speed reminders aren't sent anymore?

nic547 commented 2 years ago

Superseded by #242