From 80fdb71bd79c9634febc90c04b34c4c535a4864e Mon Sep 17 00:00:00 2001 From: Filippo Gentile Date: Wed, 12 Jun 2024 16:53:07 +0200 Subject: [PATCH 1/2] server: Z21 fix wrong parameter in message --- server/src/hardware/protocol/z21/messages.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/hardware/protocol/z21/messages.hpp b/server/src/hardware/protocol/z21/messages.hpp index 691fc065..b76cdd6b 100644 --- a/server/src/hardware/protocol/z21/messages.hpp +++ b/server/src/hardware/protocol/z21/messages.hpp @@ -391,7 +391,7 @@ static_assert(sizeof(LanXSetTrackPowerOff) == 7); // LAN_X_SET_TRACK_POWER_ON struct LanXSetTrackPowerOn : LanX { - uint8_t db0 = LAN_X_SET_TRACK_POWER_OFF; + uint8_t db0 = LAN_X_SET_TRACK_POWER_ON; uint8_t checksum = 0xa0; LanXSetTrackPowerOn() : From dd5da3ed8c09bb8dbdb2ec75ac84c021a02d9141 Mon Sep 17 00:00:00 2001 From: Filippo Gentile Date: Wed, 12 Jun 2024 18:29:18 +0200 Subject: [PATCH 2/2] server: Z21 fix setting power state - Power state must be set only once per change - If change comes from Z21 it must not be mirrored back - Z21 Emergency Stop state has implicit Power On - Sending PowerOn to Z21 is equivalent to Traintastic Run state - Sending EmergencyStop state to Z21 is equivalent to PowerOn + stopped --- .../src/hardware/interface/z21interface.cpp | 91 +++++++++++++------ .../hardware/protocol/z21/clientkernel.cpp | 38 ++++---- .../hardware/protocol/z21/clientkernel.hpp | 33 ++++--- 3 files changed, 99 insertions(+), 63 deletions(-) diff --git a/server/src/hardware/interface/z21interface.cpp b/server/src/hardware/interface/z21interface.cpp index fab29570..a3334b7e 100644 --- a/server/src/hardware/interface/z21interface.cpp +++ b/server/src/hardware/interface/z21interface.cpp @@ -209,22 +209,41 @@ bool Z21Interface::setOnline(bool& value, bool simulation) else firmwareVersion.setValueInternal(""); }); - m_kernel->setOnTrackPowerOnChanged( - [this](bool powerOn) + m_kernel->setOnTrackPowerChanged( + [this](bool powerOn, bool isStopped) { - if(powerOn == contains(m_world.state.value(), WorldState::PowerOn)) - return; - if(powerOn) - m_world.powerOn(); + { + /* NOTE: + * Setting stop and powerOn together is not an atomic operation, + * so it would trigger 2 state changes with in the middle state. + * Fortunately this does not happen because at least one of the state is already set. + * Because if we are in Run state we go to PowerOn, + * and if we are on PowerOff then we go to PowerOn. + */ + + // First of all, stop if we have to, otherwhise we might inappropiately run trains + if(isStopped && contains(m_world.state.value(), WorldState::Run)) + { + m_world.stop(); + } + else if(!contains(m_world.state.value(), WorldState::Run) && !isStopped) + { + m_world.run(); // Run trains yay! + } + + // EmergencyStop in Z21 also means power is still on + if(!contains(m_world.state.value(), WorldState::PowerOn) && isStopped) + { + m_world.powerOn(); // Just power on but keep stopped + } + } else - m_world.powerOff(); - }); - m_kernel->setOnEmergencyStop( - [this]() - { - if(contains(m_world.state.value(), WorldState::Run)) - m_world.stop(); + { + // Power off regardless of stop state + if(contains(m_world.state.value(), WorldState::PowerOn)) + m_world.powerOff(); + } }); m_kernel->setDecoderController(this); @@ -239,13 +258,18 @@ bool Z21Interface::setOnline(bool& value, bool simulation) m_kernel->setConfig(z21->config()); }); + // Avoid to set multiple power states in rapid succession if(contains(m_world.state.value(), WorldState::PowerOn)) - m_kernel->trackPowerOn(); + { + if(contains(m_world.state.value(), WorldState::Run)) + m_kernel->trackPowerOn(); // Run trains + else + m_kernel->emergencyStop(); // Emergency stop with power on + } else - m_kernel->trackPowerOff(); - - if(!contains(m_world.state.value(), WorldState::Run)) - m_kernel->emergencyStop(); + { + m_kernel->trackPowerOff(); // Stop by powering off + } Attributes::setEnabled({hostname, port}, false); } @@ -295,27 +319,42 @@ void Z21Interface::worldEvent(WorldState state, WorldEvent event) if(m_kernel) { + // Avoid to set multiple power states in rapid succession switch(event) { case WorldEvent::PowerOff: + { m_kernel->trackPowerOff(); break; - + } case WorldEvent::PowerOn: - m_kernel->trackPowerOn(); - if(!contains(state, WorldState::Run)) - m_kernel->emergencyStop(); + { + if(contains(state, WorldState::Run)) + m_kernel->trackPowerOn(); + else + m_kernel->emergencyStop(); // In Z21 E-Stop means power on but not running break; - + } case WorldEvent::Stop: - m_kernel->emergencyStop(); + { + if(contains(state, WorldState::PowerOn)) + { + // In Z21 E-Stop means power is on but trains are not running + m_kernel->emergencyStop(); + } + else + { + // This Stops everything by removing power + m_kernel->trackPowerOff(); + } break; - + } case WorldEvent::Run: + { if(contains(state, WorldState::PowerOn)) m_kernel->trackPowerOn(); break; - + } default: break; } diff --git a/server/src/hardware/protocol/z21/clientkernel.cpp b/server/src/hardware/protocol/z21/clientkernel.cpp index d78a0892..7deee983 100644 --- a/server/src/hardware/protocol/z21/clientkernel.cpp +++ b/server/src/hardware/protocol/z21/clientkernel.cpp @@ -113,8 +113,9 @@ void ClientKernel::receive(const Message& message) if(m_trackPowerOn != TriState::False) { m_trackPowerOn = TriState::False; - if(m_onTrackPowerOnChanged) - m_onTrackPowerOnChanged(false); + m_emergencyStop = TriState::False; + if(m_onTrackPowerChanged) + m_onTrackPowerChanged(false, false); } }); } @@ -123,11 +124,12 @@ void ClientKernel::receive(const Message& message) EventLoop::call( [this]() { - if(m_trackPowerOn != TriState::True) + if(m_trackPowerOn != TriState::True || m_emergencyStop != TriState::False) { m_trackPowerOn = TriState::True; - if(m_onTrackPowerOnChanged) - m_onTrackPowerOnChanged(true); + m_emergencyStop = TriState::False; + if(m_onTrackPowerChanged) + m_onTrackPowerChanged(true, false); } }); } @@ -142,8 +144,10 @@ void ClientKernel::receive(const Message& message) if(m_emergencyStop != TriState::True) { m_emergencyStop = TriState::True; - if(m_onEmergencyStop) - m_onEmergencyStop(); + m_trackPowerOn = TriState::True; + + if(m_onTrackPowerChanged) + m_onTrackPowerChanged(true, true); } }); } @@ -445,20 +449,14 @@ void ClientKernel::receive(const Message& message) EventLoop::call([this, trackPowerOn, stopState]() { - if(m_trackPowerOn != trackPowerOn) + if(m_trackPowerOn != trackPowerOn || m_emergencyStop != stopState) { m_trackPowerOn = trackPowerOn; - if(m_onTrackPowerOnChanged) - m_onTrackPowerOnChanged(trackPowerOn == TriState::True); - } - - if(m_emergencyStop != stopState) - { m_emergencyStop = stopState; - if(m_onEmergencyStop && m_emergencyStop == TriState::True) - { - m_onEmergencyStop(); - } + + if(m_onTrackPowerChanged) + m_onTrackPowerChanged(trackPowerOn == TriState::True, + stopState == TriState::True); } }); } @@ -504,7 +502,7 @@ void ClientKernel::trackPowerOff() { assert(isEventLoopThread()); - if(m_trackPowerOn != TriState::False) + if(m_trackPowerOn != TriState::False || m_emergencyStop != TriState::False) { m_ioContext.post( [this]() @@ -518,7 +516,7 @@ void ClientKernel::emergencyStop() { assert(isEventLoopThread()); - if(m_emergencyStop != TriState::True) + if(m_trackPowerOn != TriState::True || m_emergencyStop != TriState::True) { m_ioContext.post( [this]() diff --git a/server/src/hardware/protocol/z21/clientkernel.hpp b/server/src/hardware/protocol/z21/clientkernel.hpp index 03a13919..29dad86d 100644 --- a/server/src/hardware/protocol/z21/clientkernel.hpp +++ b/server/src/hardware/protocol/z21/clientkernel.hpp @@ -94,7 +94,7 @@ class ClientKernel final : public Kernel * * \sa EventLoop */ - TriState m_trackPowerOn; + TriState m_trackPowerOn = TriState::Undefined; /*! * \brief m_emergencyStop caches command station emergency stop state. @@ -104,9 +104,19 @@ class ClientKernel final : public Kernel * * \sa EventLoop */ - TriState m_emergencyStop; - std::function m_onTrackPowerOnChanged; - std::function m_onEmergencyStop; + TriState m_emergencyStop = TriState::Undefined; + + /*! + * \brief m_onTrackPowerChanged callback is called when Z21 power state changes. + * + * \note It is always called from event loop thread + * \note First argument is powerOn, second argument is isStopped + * In Z21 EmergencyStop is really PowerOn + EmergencyStop and + * PowerOn implicitly means Run so we cannot call \sa trackPowerOn() if world must be stopped + * + * \sa EventLoop + */ + std::function m_onTrackPowerChanged; DecoderController* m_decoderController = nullptr; @@ -222,10 +232,10 @@ public: * @param[in] callback ... * @note This function may not be called when the kernel is running. */ - inline void setOnTrackPowerOnChanged(std::function callback) + inline void setOnTrackPowerChanged(std::function callback) { assert(!m_started); - m_onTrackPowerOnChanged = std::move(callback); + m_onTrackPowerChanged = std::move(callback); } /** @@ -240,17 +250,6 @@ public: */ void emergencyStop(); - /** - * @brief ... - * @param[in] callback ... - * @note This function may not be called when the kernel is running. - */ - inline void setOnEmergencyStop(std::function callback) - { - assert(!m_started); - m_onEmergencyStop = std::move(callback); - } - /** * @brief Set the decoder controller * @param[in] decoderController The decoder controller