Train: new active property

[gfgit]

It is mirrored in RailVehicle activeTrain property.

Activating a Train should be an atomic operation as it must check for every contained vehicles to be free and then proceed.

See discussion: #26

TODO:

• Ensure activate Train is atomic (i.e. no concurrent activate from different clients)
• An Inactive train should not move so prevent setting speed if not active and keep active until speed reaches zero
• Setting speed on a RailVehicle should set back train speed if it's inside an active train
• A train cannot be activated if any of it's components is already running
• Add activate train button to train list widget near throttle and maybe also inside throttle widget
• Feedback to user when failing to activate Train
• Only active trains propagate emergency stop to decoders
• When deactivate while in EStop, set speed to zero to all decoders?
• Add translations
• Clean commits and rebase on master

[gfgit]

Maybe we should move active property handler to a member function instead of lambda inside constructor

[reinder]

Nice summary and checklist :)

Train activation runs in the event thread (almost all calls do), they are processed one by one, they don't run parallel. So you can check if activation is possible before setting the activeTrain on each RailVehicle. Setting and then rolling back causes change events to be fired twice for those set and reset.

p.s.1. vehicles supports range based for loops: for(const auto& vehicle : *vehicles)
p.s.2. I really need to write some documentation about Traintastic internal architecture #notetoself

[gfgit]

Sorry, not building because I called "bind" in a wrong way.
I have problems with disabling train throttle while not active. It keeps disabled even after activating

[reinder]

Sorry, not building because I called "bind" in a wrong way.

No problem, that is what the action is for, to check those things :)

I have problems with disabling train throttle while not active. It keeps disabled even after activating

To be able to use Attibutes::setEnabled on a property, the attribute must be added in the constructor using Attibutes::addEnabled.

The client side throttle widget must be modified too, it can't handle that at the moment because it wasn't used until now.

[gfgit]

The client side throttle widget must be modified too, it can't handle that at the moment because it wasn't used until now.

I was going for forcing speed to zero and discarding speed changes from clients.
This way no modification to widget is needed.
Enabling/disabling migth be a cleaner way of doibg this maybe, but I would still Force value to zero.

The problem I'm having is that The Train is stuck in Accelerating state and updateSpeed is not called from property change so the speed is always zero after train is activated

[reinder]

The client side throttle widget must be modified too, it can't handle that at the moment because it wasn't used until now.

I was going for forcing speed to zero and discarding speed changes from clients. This way no modification to widget is needed. Enabling/disabling migth be a cleaner way of doibg this maybe, but I would still Force value to zero.

Disabled properties don't accept setting a value (unless setValueInternal is used), all client network calls and lua script call use the setValue route which checks the enable.

It would also be nice to use the enable state to inform the user that the throttle is disabled. Sending commands to the server is useless.

The problem I'm having is that The Train is stuck in Accelerating state and updateSpeed is not called from property change so the speed is always zero after train is activated

Don't know how it is implemented now, but be aware that using setValueInternal bypasses the onChanged and onSet callbacks that were set in the constructor.

[gfgit]

The point is if I call updateSpeed() from setTrainActive(), the "active" property still has the old value.
I will try also the "changed" handler instead of "set"

EDIT: changed handler fixes the issue

But it's not working properly yet. "isStopped" property is true even when running.
When hitting emergency stop, train speed drops to zero but single decoders still have some speed steps set. So it gives the impression that train can be deactivated because it is not moving but it fails because decoders are not at zero speed

[reinder]

The set handler verfies the new value, if it returns true the property value is updated and then the changed handler is fired.

I think we should build soms test, that makes it easier to check all situations, the EventLoop needs to run to, that may be a bit tricky.

[reinder]

isStopped wasn't always correct, should be fixed now @ master.

[gfgit]

Squashed 2 commits together, rebased on current master and force pushed

[gfgit]

Hi, how do we avoid infinite loop of Train set speed -> decoder changes speed -> update train speed?
I guess just comparing values is not sufficient due to the speed step rounding and float epsilon for equality tests.
My first idea is to add a bool guard "insideSetSpeed" in Train::setSpeed() which calls PoweredRailVehicle which inside change handler checks the guard bool and updates Train speed only if guard value is false.
It seems a bit hacky...

[gfgit]

Strange enough I do not get infinite recursion...
But the throttle is all wrong, it does not get to zero if I change speed from decoder, and sometimes it does the opposite of what you want.
With the boolean guard things get a little better but there is a decoder throttle change triggered by Client::processMessage() which, being queued, happens after bool guard is already reset to false. This way I cannot break recursion efficiently but I don't understand this client message where it comes from

[reinder]

In server/src/vehicle/rail/poweredrailvehicle.cpp:

  propertyChanged.connect(
    [this](BaseProperty &prop)
    {
      if(prop.name() == "decoder")
        registerDecoder();
    });

If we need this I suggest to add a virtual void decoderChanged() {} method to RailVehicle and set it as changed handler for the decoder property, it can can be overriden by PoweredRailVehicle. This is also used for the IdObject::id property.

About handling other speed changes
That is indeed a bit tricky. Setting the decoder throttle value to the train throttleSpeed can give issues, as the train throttleSpeed is the target speed. e.g. train throttleSpeed can be 100 km/h, while accelerating it constantly updates the decoder throttle, so that should not update the train throttleSpeed then.

When using a handheld controller, e.g. WiThrottle or WLANmaus, those commands are now send to the decoder, if we make the decoder aware of the vehicle it is connected to we can easily redirect the commands via the vehicle to the active train. (If the vehicle is in only one train and it isn't active we can even try to activate it....but that won't work in all cases so it might be confusing for the user). Another reason to make the decoder aware of the vehicle is that we currently can assign a decoder to multiple vehicles....not good.

What about direction changed? Estop?

On my wishlist is also a train mode property, it can be set to:

• Manual: full control by the user
• Manual w/ protection: Controlled by a user, but monitored by Traintastic for speed limits, signals, direction limit etc. (maybe multiple levels: a strict level, just limit it or warn user, no action -> estop)
• Automatic: fully controlled by Traintastic

Maybe we need something like this already.

Please share your thought :) Think we need no walk troughs different scenarios to find a solution and maybe add some setting so the user can choose the preferred behavior.

[reinder]

Strange enough I do not get infinite recursion... But the throttle is all wrong, it does not get to zero if I change speed from decoder, and sometimes it does the opposite of what you want. With the boolean guard things get a little better but there is a decoder throttle change triggered by Client::processMessage() which, being queued, happens after bool guard is already reset to false. This way I cannot break recursion efficiently but I don't understand this client message where it comes from

How do you alter the Decoder?

p.s. I posted my message before I read this one.

[gfgit]

How do you alter the Decoder?

I have committed all the changes. I did not touch Client code, only Train and PoweredVehicle on Server side.
What seems to happen is:

1. User changes Decoder throttle
2. Handler calculates km/h and changes Train throttle speed
3. Train speed is recalculated, Train::setSpeed() gets called (insideSetSpeed = true)
4. PoweredRailVehicle handles the speed step change but is no-op because of bool guard
5. Train::setSpeed() ends (insideSetSpeed = false)
6. Client::processMessage() triggers a property change on decoder throttle
7. PoweredRailVehicle handles the speed step change. Now bool guard is false so it calls Train::setSpeed() which should not happen because we already synced speeds

The only possible reason I can imagine is that by scrolling throttle widget with mouse, multiple speed changes are sent but because server debugger hits a breaking point, it handles only the first (until point 5) and then reads the client queue and goes on (point 6, which is like point 1)

[reinder]

Scrolling throttle widget with mouse can indeed send multiple messages, I'd expect one per "tick".

The decoder throttle was temporary (because Trains didn't have them), so that should be removed. We should also test with an external throttle, e.g. Z21 app via WLANmaus or EngineDriver (Android) / WiThrottle (iOS) via WiThrottle.

I'll try to look into it tomorrow further. Real hardware is also tricky as is has a response time and the response is handled async so the boolean guard is the gone already.

[gfgit]

If we need this I suggest to add a virtual void decoderChanged() {} method to RailVehicle

Yes I agree, this was a quick test so I used the signals.

train throttleSpeed can be 100 km/h, while accelerating it constantly updates the decoder throttle, so that should not update the train throttleSpeed then.

This is correct. Let's say I have 2 locomotives and one of them has slow acceleration (and Traintastic is aware of that somehow), it can make train acceleration slower to avoid temporary speed mismatch while the 2 locomotives accelerate because the faster one reaches the target speed sooner.
When controlling the faster one from a physical throttle (say MultiMaus) I think user sets a new speed which becomes target speed. Then traintastic, knowing it cannot be reached immediately lowers the decoder speed according to Train acceleration curve. So user should see the speed throttle having a spike to target speed, then go back to original speed and then slowly raise to target speed again.

if we make the decoder aware of the vehicle it is connected to we can easily redirect the commands via the vehicle to the active train

I'm not confident on this. It seems not really elegant. Decoder is abstract and I think it can be associated also to a turnout.
Some trains have 2 decoders with same address to repeat lights on rear of passenger car. Some other can have 2 decoders for same vehicle to get extra functions...
But I see your point.
I don't think this would solve the issue because we have to account external throttle changes (so not WiThrottle or WLANMaus which are handled differently). When Train is accelerating, we get constant throttle changes on the command station bus (e.g. XpressNet) which we should not react to, because they were originated by us.
While if the change was made by an external throttle, we should react to it and sync train speed.

This is the same recursion loop seen above but it involves different components (not only software this time) so it's even worse because we cannot use the bool guard trick on async communication with the command station.
If we solve this complex case, then it should work regardless of decoder being aware of rail vehicle and regardless of "internal" recursion loop (which still could be short circuited to be faster).

What about direction changed? Estop?

I've implemented Estop but not direction change yet.
Also in a Train, not all vehicles must be on same physical direction. So forward for one vehicle could be backward for another.
And speed curves can be asymmetrical...

On my wishlist is also a train mode property

This is very cool. It would allow background automatic traffic while manually driving a train and collisions are avoided by signal system and automatic trains could adjust the route based on manual trains getting in their way.
Like for example if in a station there are many free platforms and an automatic train is set to stop at platform 5 but it's occupied by a manually driven train, traintastic could choose to wait a bit but then make it stop to platform 6 (provided it's reachable from same position) and maybe generate a message to inform of schedule variation.
But this is still dreaming...

[gfgit]

We should also test with an external throttle

About this I would not consider WiThrottle "external" if connected directly to Traintastic.
If we are providing the server we can also cheat on object state, like intercepting user input and modifying it (for example limit speed under a certain level). Worst can happen is mobile UI not matching Traintastic state but the correct value is used by Traintastic.

This is different from a real command station which keeps inside an internal record of decoder states, so we cannot cheat on object state because the effective value is decided by the command station. To intercept we can only quickly tell the command station to alter the state of a specific object, differently from what user has just set.
This means there is a small time period in which the user chosen value is applied before the command station reacts to Traintastic "fixing" the value.

[gfgit]

I had an idea.
We record the last decoder step set by Train in each decoder. Because the value is floating point it should be compared with some epsilon (also Train speed should be compared with epsilon).
In case of finite number of steps (like 14/28/128 steps), the value should be rounded to the speed step and then compared.
This way, if decoder handler reacts to speed step change but it's equal to "lastSpeedSetByTrain" it return without further operation. This should work also for async messages. If user changes the speed while Train is also changing the speed (because of acceleration), we detect it by decoder step mismatch and trigger Train speed adjustment which will change "lastSpeedSetByTrain" and begin the process again.
A problem could arise when user throttle is sending many intermediate values while rotating, and Train acceleration is already triggered at first speed change sent.

[gfgit]

This second approach seems to work but suffers of rounding errors.

TRAIN new target speed
TRAIN update speed, current: 0 km/h target: 2 km/h
TRAIN new speed: 0.36 km/h
VEHICLE: new speed step: 0.36 km/h --> 0.0078125
VEHICLE: same speed step: 0.0078125 0.0078125
TRAIN update speed, current: 0.36 km/h target: 2 km/h
TRAIN new speed: 0.72 km/h
VEHICLE: new speed step: 0.72 km/h --> 0.015625
VEHICLE: same speed step: 0.015625 0.015625



TRAIN new target speed
TRAIN update speed, current: 0.72 km/h target: 4 km/h
TRAIN new speed: 1.08 km/h
VEHICLE: new speed step: 1.08 km/h --> 0.0234375
VEHICLE: same speed step: 0.0234375 0.0234375
TRAIN update speed, current: 1.08 km/h target: 4 km/h
TRAIN new speed: 1.44 km/h
VEHICLE: new speed step: 1.44 km/h --> 0.03125
VEHICLE: same speed step: 0.03125 0.03125
TRAIN update speed, current: 1.44 km/h target: 4 km/h
TRAIN new speed: 1.8 km/h
VEHICLE: new speed step: 1.8 km/h --> 0.0390625
VEHICLE: same speed step: 0.0390625 0.0390625
TRAIN update speed, current: 1.8 km/h target: 4 km/h
TRAIN new speed: 2.16 km/h
VEHICLE: new speed step: 2.16 km/h --> 0.046875
VEHICLE: same speed step: 0.046875 0.046875
TRAIN update speed, current: 2.16 km/h target: 4 km/h
TRAIN new speed: 2.52 km/h
VEHICLE: new speed step: 2.52 km/h --> 0.046875
TRAIN update speed, current: 2.52 km/h target: 4 km/h
TRAIN new speed: 2.88 km/h
VEHICLE: new speed step: 2.88 km/h --> 0.0546875
VEHICLE: same speed step: 0.0546875 0.0546875
TRAIN update speed, current: 2.88 km/h target: 4 km/h
TRAIN new speed: 3.24 km/h
VEHICLE: new speed step: 3.24 km/h --> 0.0625
VEHICLE: same speed step: 0.0625 0.0625
TRAIN update speed, current: 3.24 km/h target: 4 km/h
TRAIN new speed: 3.6 km/h
VEHICLE: new speed step: 3.6 km/h --> 0.0703125
VEHICLE: same speed step: 0.0703125 0.0703125
TRAIN update speed, current: 3.6 km/h target: 4 km/h
TRAIN new speed: 3.96 km/h
VEHICLE: new speed step: 3.96 km/h --> 0.078125
VEHICLE: same speed step: 0.078125 0.078125
TRAIN update speed, current: 3.96 km/h target: 4 km/h
TRAIN new speed: 4 km/h
VEHICLE: new speed step: 4 km/h --> 0.078125



VEHICLE: new train speed: 3.40625 km/h --> 0.068125                                  (NOTE 1)



TRAIN new target speed
TRAIN update speed, current: 4 km/h target: 3.40625 km/h
TRAIN new speed: 3.82 km/h
VEHICLE: new speed step: 3.82 km/h --> 0.078125
VEHICLE: same speed step: 0.078125 0.078125
TRAIN update speed, current: 3.82 km/h target: 3.40625 km/h
TRAIN new speed: 3.64 km/h
VEHICLE: new speed step: 3.64 km/h --> 0.0703125
VEHICLE: same speed step: 0.0703125 0.0703125
TRAIN update speed, current: 3.64 km/h target: 3.40625 km/h
TRAIN new speed: 3.46 km/h
VEHICLE: new speed step: 3.46 km/h --> 0.0703125
TRAIN update speed, current: 3.46 km/h target: 3.40625 km/h
TRAIN new speed: 3.40625 km/h
VEHICLE: new speed step: 3.40625 km/h --> 0.0703125                                 (NOTE 2)

Here I have a train with a single powered vehicle with max speed 50 km/h
I've put log in:

• Train::throttleSpeed changed handler: "TRAIN: new target speed"
• Train::updateSpeed() at beggining: "TRAIN update speed, current: 0 km/h target: 2 km/h"
• Train::setSpeed(): "TRAIN new speed: 0.36 km/h"
• PoweredRailVehicle::setSpeed(): "VEHICLE: new speed step: 3.24 km/h --> 0.0625"
• PoweredRailVehicle's decoder throttle change handler: "VEHICLE: same speed step: 0.015625 0.015625" (if change comes from Train) or "VEHICLE: new train speed: 3.40625 km/h --> 0.068125" (change comes from decoder throttle -> direct user input)

You can easily see that when user sets a new speed directly from decoder throttle (NOTE 1), it is then reset to original value and follows the acceleration curve but end value is a bit different (NOTE 2)
Original user input is step 0.068125 which corresponds to 3.40625 km/h while the step calculated for same speed is 0.0703125

NOTE 3: the decoder is at 128 steps, the first value is step 8.72 (not possible) while final value is exactly step 9 which is correct.
So this means decoder throttle send floating value for step which does not respect decoder step precision (128 steps) while train calculated speed rounds to the correct speed step at cost of having slightly lower/higher vehicle speed

[reinder]

This is correct. Let's say I have 2 locomotives and one of them has slow acceleration (and Traintastic is aware of that somehow), it can make train acceleration slower to avoid temporary speed mismatch while the 2 locomotives accelerate because the faster one reaches the target speed sooner.

Acceleration is determined by the train, for smooth operation it is best that the decoder doesn't do any braking/acceleration by itself. If Traintastic has to take that into account to it becomes much harder. Most decoders nowadays have a function to disable those braking/acceleration curves. (And that's why Traintastic has an always on/off option for functions :))

I'm not confident on this. It seems not really elegant. Decoder is abstract and I think it can be associated also to a turnout.
Some trains have 2 decoders with same address to repeat lights on rear of passenger car. Some other can have 2 decoders for same vehicle to get extra functions...

The Decoder class is for locomotive decoders, accessorydecoder outputs are handled by the Output class. (To make that more clear it would be better to call it LocomotiveDecoder.)
If a locomotive/multiple unit has multiple decoders with the same address, that's fine, but Traintastic doesn't need to know :)
If a locomotive has a secondary decoder (or one decoder with multiple addresses), then Traintastic needs to know. We can add an aditional property for that. For the speed we can just send it to the primary decoder only (or add an option to send it to both)

We need at least something to make sure it isn't possible to "plug" the same decoder in multiple powered vehicles, that will give all kind of weird issues.

Also in a Train, not all vehicles must be on same physical direction. So forward for one vehicle could be backward for another.

For the direction we should test the value against the decoder it is received in, if it matches, do nothing, else toggle the train direction. That will then correct the other powered vehicle directions.

This is very cool. It would allow background automatic traffic while manually driving a train and collisions are avoided by signal system and automatic trains could adjust the route based on manual trains getting in their way.
Like for example if in a station there are many free platforms and an automatic train is set to stop at platform 5 but it's occupied by a manually driven train, traintastic could choose to wait a bit but then make it stop to platform 6 (provided it's reachable from same position) and maybe generate a message to inform of schedule variation.
But this is still dreaming...

One day we will get there :)

This is different from a real command station which keeps inside an internal record of decoder states, so we cannot cheat on object state because the effective value is decided by the command station. To intercept we can only quickly tell the command station to alter the state of a specific object, differently from what user has just set.
This means there is a small time period in which the user chosen value is applied before the command station reacts to Traintastic "fixing" the value.

I agree, the WiThrottle etc. is easier to handle because the command station isn't involved yet. So we can do that later :) Their implementation is also a bit different, so that needs to be corrected too.

I had an idea.
We record the last decoder step set by Train in each decoder. Because the value is floating point it should be compared with some epsilon (also Train speed should be compared with epsilon).
In case of finite number of steps (like 14/28/128 steps), the value should be rounded to the speed step and then compared.
This way, if decoder handler reacts to speed step change but it's equal to "lastSpeedSetByTrain" it return without further operation. This should work also for async messages. If user changes the speed while Train is also changing the speed (because of acceleration), we detect it by decoder step mismatch and trigger Train speed adjustment which will change "lastSpeedSetByTrain" and begin the process again.
A problem could arise when user throttle is sending many intermediate values while rotating, and Train acceleration is already triggered at first speed change sent.

Mapping it to speeds steps can be tricky:

• XpressNet for example support direct speed step control, 14/27/28/128. You can control it from Traintastic.
• LocoNet always uses 126 steps in the messages, but as far as I know there is no way to determine wat the command station will do with the DCC on the track.
• DCC++EX uses 28 or 128 for all addresses, mixing is not (yet) possible.
• Marklin CS2 uses 1000 steps in the messages
• ESU ECoS has direct speedstep control

e.g. LocoNet, see here corrects the throttle without epsilon, that will cause issues. Because of the 126 steps it should use an epsilon based on the, so if the throttle is LocoNet step 9.73 which will be send as 10 then it shouldn't update the throttle value to 10 but keep 9.73 because it is within the bandwidth. If something on LocoNet changes it to 11 the it must update the throttle value to 11 / 126. I bought a 2nd hand LocoNet throttle (FRED) some time ago....didn't try it yet.

TRAIN new target speed ... etc.

Nice analysis and test run, I like it :) I wonder why the is a difference at the end, the train should always correct the value to the target speed for the final step.

If the decoder throttle is set by something else than the train, should it not just set the train speed accordingly and bypass the acceleration/braking logic? I think Traintastic then has to asume that something else is "in control" and just accept the speed. (Might change when mode is added in the future)

[gfgit]

For the direction we should test the value against the decoder it is received in, if it matches, do nothing, else toggle the train direction. That will then correct the other powered vehicle directions.

Lets say you have vehicle A and B in a train. Then you break the train, run B trough a reverse loop and couple back to A in the same train or a new one. Now B has swapped direction compared to A.

Nice analysis and test run, I like it :) I wonder why the is a difference at the end, the train should always correct the value to the target speed for the final step.

I've added a new DeoderThrottleProperty which rounds values to the nearest decder step and is updated when decoder max steps change. I don't know if I already pushed it. It's not yet working properly because decoder throttle shlws full speedometer. And you hardly see the difference with the percentage spees. See #48

[reinder]

You not yet pushed it :) I'd like to see what you made :)

[gfgit]

I'm not sure it's the rigth approach. If decoder is set to 128 steps it gets rounded to it but then you send through Loconet which has 126 steps and all gets scaled with loss of precision.
Maybe it's better to keep the original value.
I'll push anyway, we will test a bit and then decide for keeping or removing

[reinder]

Modifying values when setting a property can be done in the onSet handler, it receives the value by ref. So you can modify the value and return true to accept it. If it then is identical to the current value it won't fire an onChanged.
Minimum and maximum are automatically handled if these attributes are set. The only option to set a value outside the min...max range is using setValueInternal, that function bypassed all built in checks.

I think it is best to leave the value untouched as long as it is in the step bandwidth of the interface/decoder.

I really need to do some test with a command station and a roller bench this week.

[reinder]

@gfgit can we split this PR into one for the active train stuff and one for the speed update from RailVehicle?

Setup my rollerbench with two locomotives, driving a train with two engines works fine. Next step is to experiment with spped/direction changes by throttles on the command station (I use a Digikeijs DR5000 via LocoNet). For that the active train part is needed :)

[gfgit]

@gfgit can we split this PR into one for the active train stuff and one for the speed update from RailVehicle?

Done, work continues in #54