Skip to content

Commit

Permalink
Adds local loopback to TractionThrottle. (#756)
Browse files Browse the repository at this point in the history 
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.

===

* Adds local loopback to TractionThrottle.

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.

* Remove unnecessary verbose logging.

* Adds extra nodes to the test fixture. Removes repeated boilerplate for adding the second train node.

* Adds a test for multiple throttle objects listening to the same train.

* Adds more test cases to the local listener feedback.
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.

* Adds test coverage for the last throttle removing the consist link.

* Makes sure we hold on to a ref while sending the packet to the send flow.
  • Loading branch information
@balazsracz
balazsracz authored Jan 27, 2024
1 parent ad0681b commit 5c1f88a
Show file tree
Hide file tree
Showing 3 changed files with 354 additions and 54 deletions.
14 changes: 7 additions & 7 deletions src/openlcb/TractionClient.hxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,16 +153,16 @@ private:

Action entry() OVERRIDE
{
LOG_ERROR("response came");
LOG(VERBOSE, "response came");

if (!trigger_)
{
// We already matched -- drop all packets to the floor.
LOG_ERROR("no trigger");
LOG(VERBOSE, "no trigger");
return release_and_exit();
}
if (nmsg()->dstNode != expectedDst_) {
LOG_ERROR("dst not match");
LOG(VERBOSE, "dst not match");
return release_and_exit();
}
/// @TODO(balazs.racz) factor out this code into a helper function that
Expand All @@ -172,15 +172,15 @@ private:
{
if (expectedSrc_.id != nmsg()->src.id)
{
LOG_ERROR("src.id not match");
LOG(VERBOSE, "src.id not match");
return release_and_exit();
}
}
else if (expectedSrc_.alias && nmsg()->src.alias)
{
if (expectedSrc_.alias != nmsg()->src.alias)
{
LOG_ERROR("src.alias not match");
LOG(VERBOSE, "src.alias not match");
return release_and_exit();
}
}
Expand All @@ -194,11 +194,11 @@ private:
// Now: message is from the right source node, to the right destination
// node.
if (nmsg()->payload.size() < 1) {
LOG_ERROR("no payload");
LOG(VERBOSE, "no payload");
return release_and_exit();
}
if (nmsg()->payload[0] != expectedType_) {
LOG_ERROR("payload type no match");
LOG(VERBOSE, "payload type no match");
return release_and_exit();
}
// Now: we matched!
Expand Down
202 changes: 176 additions & 26 deletions src/openlcb/TractionThrottle.cxxtest
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,22 +8,37 @@ namespace openlcb
{

static constexpr NodeID TRAIN_NODE_ID = 0x06010000C000 | 1372;
static constexpr NodeID REMOTE_NODE_ID = 0x0501010118F3ull;

class ThrottleTest : public AsyncNodeTest
{
protected:
ThrottleTest()
{
print_all_packets();
run_x(
[this]() { otherIf_.local_aliases()->add(TRAIN_NODE_ID, 0x771); });
run_x([this]() {
otherIf_.local_aliases()->add(TRAIN_NODE_ID, 0x771);
otherIf_.local_aliases()->add(TRAIN_NODE_ID+1, 0x772);
otherIf_.local_aliases()->add(REMOTE_NODE_ID, 0x553);
});
trainNode_.reset(new TrainNodeForProxy(&trainService_, &trainImpl_));
trainNode2_.reset(new TrainNodeForProxy(&trainService_, &trainImpl2_));
remoteNode_.reset(new DefaultNode(&otherIf_, REMOTE_NODE_ID, true));
wait();
// This will re-fill all the alias caches.
send_packet(":X19490559N;");
send_packet(":X10702559N;");
wait();
}

LoggingTrain trainImpl_{1372};
std::unique_ptr<TrainNode> trainNode_;

LoggingTrain trainImpl2_{1373};
std::unique_ptr<TrainNode> trainNode2_;

std::unique_ptr<DefaultNode> remoteNode_;

IfCan otherIf_{&g_executor, &can_hub0, 5, 5, 5};
TrainService trainService_{&otherIf_};

Expand Down Expand Up @@ -423,22 +438,16 @@ TEST_F(ThrottleClientTest, ReassignWithListener)
EXPECT_EQ(1, trainNode_->query_consist_length());
EXPECT_QRYCONSIST(node_->node_id(), 0x8C, 0);

static auto TRAIN_NODE_ID2 = TRAIN_NODE_ID + 1;
run_x([this]() { otherIf_.local_aliases()->add(TRAIN_NODE_ID2, 0x772); });
LoggingTrain train_impl2{1373};
TrainNodeForProxy node2(&trainService_, &train_impl2);
wait();

b = invoke_flow(&throttle_, TractionThrottleCommands::ASSIGN_TRAIN,
TRAIN_NODE_ID2, true);
TRAIN_NODE_ID + 1, true);
ASSERT_EQ(0, b->data()->resultCode);
EXPECT_EQ(0, trainNode_->query_consist_length());
EXPECT_EQ(1, node2.query_consist_length());
EXPECT_EQ(1, trainNode2_->query_consist_length());

b = invoke_flow(&throttle_, TractionThrottleCommands::RELEASE_TRAIN);
ASSERT_EQ(0, b->data()->resultCode);
EXPECT_EQ(0, trainNode_->query_consist_length());
EXPECT_EQ(0, node2.query_consist_length());
EXPECT_EQ(0, trainNode2_->query_consist_length());

wait();
}
Expand All @@ -453,17 +462,11 @@ TEST_F(ThrottleClientTest, ReassignWithoutListener)

EXPECT_EQ(1, trainNode_->query_consist_length());

static auto TRAIN_NODE_ID2 = TRAIN_NODE_ID + 1;
run_x([this]() { otherIf_.local_aliases()->add(TRAIN_NODE_ID2, 0x772); });
LoggingTrain train_impl2{1373};
TrainNodeForProxy node2(&trainService_, &train_impl2);
wait();

b = invoke_flow(&throttle_, TractionThrottleCommands::ASSIGN_TRAIN,
TRAIN_NODE_ID2, false);
TRAIN_NODE_ID + 1, false);
ASSERT_EQ(0, b->data()->resultCode);
EXPECT_EQ(0, trainNode_->query_consist_length());
EXPECT_EQ(0, node2.query_consist_length());
EXPECT_EQ(0, trainNode2_->query_consist_length());

wait();
}
Expand Down Expand Up @@ -521,12 +524,6 @@ TEST_F(ThrottleClientTest, HeartbeatWrongTrain)
TRAIN_NODE_ID, false);
ASSERT_EQ(0, b->data()->resultCode);

static auto TRAIN_NODE_ID2 = TRAIN_NODE_ID + 1;
run_x([this]() { otherIf_.local_aliases()->add(TRAIN_NODE_ID2, 0x772); });
LoggingTrain train_impl2{1373};
TrainNodeForProxy node2(&trainService_, &train_impl2);
wait();

// Primes the caches.
openlcb::send_message(trainNode_.get(), Defs::MTI_TRACTION_CONTROL_COMMAND,
NodeHandle(node_->node_id()), TractionDefs::fn_set_payload(13, 1));
Expand All @@ -536,7 +533,7 @@ TEST_F(ThrottleClientTest, HeartbeatWrongTrain)
// heartbeat request is sent from wrong train to throttle
expect_packet(":X191E9772N022A400303;");
// no response from the throttle.
openlcb::send_message(&node2, Defs::MTI_TRACTION_CONTROL_REPLY,
openlcb::send_message(trainNode2_.get(), Defs::MTI_TRACTION_CONTROL_REPLY,
NodeHandle(node_->node_id()),
TractionDefs::heartbeat_request_payload());
wait();
Expand Down Expand Up @@ -627,4 +624,157 @@ TEST_F(ThrottleClientTest, ListenerCallback)
EXPECT_FALSE(throttle_.get_emergencystop());
}


TEST_F(ThrottleClientTest, MultiListener)
{
TractionThrottle throttle2{node_};
TractionThrottle throttler{remoteNode_.get()};
StrictMock<MockListener> l, l2, lr;

throttle_.set_throttle_listener(
std::bind(&ListenerInterface::update, &l, std::placeholders::_1));
throttle2.set_throttle_listener(
std::bind(&ListenerInterface::update, &l2, std::placeholders::_1));
throttler.set_throttle_listener(
std::bind(&ListenerInterface::update, &lr, std::placeholders::_1));

auto b = invoke_flow(&throttle_, TractionThrottleCommands::ASSIGN_TRAIN,
TRAIN_NODE_ID, true);
ASSERT_EQ(0, b->data()->resultCode);
b = invoke_flow(&throttle2, TractionThrottleCommands::ASSIGN_TRAIN,
TRAIN_NODE_ID, true);
ASSERT_EQ(0, b->data()->resultCode);
b = invoke_flow(&throttler, TractionThrottleCommands::ASSIGN_TRAIN,
TRAIN_NODE_ID, true);
ASSERT_EQ(0, b->data()->resultCode);

wait();

// This message will trigger no callback on the bus, but the local feedback
// will make it appear. The remote node will get its own feedback.
EXPECT_CALL(l, update(23));
EXPECT_CALL(lr, update(23));
throttle2.set_fn(23, 1);
wait();
Mock::VerifyAndClear(&l);
Mock::VerifyAndClear(&l2);
Mock::VerifyAndClear(&lr);

// If the remote node calls, one message will come on the bus but both
// throttles will call update.
EXPECT_CALL(l, update(22));
EXPECT_CALL(l2, update(22));
throttler.set_fn(22, 1);
wait();
Mock::VerifyAndClear(&l);
Mock::VerifyAndClear(&l2);
Mock::VerifyAndClear(&lr);

// If a third node calls, all three will call update.
EXPECT_CALL(l, update(15));
EXPECT_CALL(l2, update(15));
EXPECT_CALL(lr, update(15));
send_packet(":X195EB330N07710100000f0001;"); // fn 15 = 1
wait();
Mock::VerifyAndClear(&l);
Mock::VerifyAndClear(&l2);
Mock::VerifyAndClear(&lr);

LOG(INFO, "switching throttle2 to other train.");

// =================
// Local second throttle will be switched over to a different train.
b = invoke_flow(&throttle2, TractionThrottleCommands::ASSIGN_TRAIN,
TRAIN_NODE_ID + 1, true);
ASSERT_EQ(0, b->data()->resultCode);

// Now a third node calls, two shall call update.
EXPECT_CALL(l, update(14));
EXPECT_CALL(lr, update(14));
send_packet(":X195EB330N07710100000e0001;"); // fn 14 = 1
wait();
Mock::VerifyAndClear(&l);
Mock::VerifyAndClear(&l2);
Mock::VerifyAndClear(&lr);

// When the remote node calls, the local shall update.
EXPECT_CALL(l, update(13));
throttler.set_fn(13, 1);
wait();
Mock::VerifyAndClear(&l);
Mock::VerifyAndClear(&l2);
Mock::VerifyAndClear(&lr);

// Local2 calls go to a different train, no update callbacks.
throttle2.set_fn(13, 1);
wait();
Mock::VerifyAndClear(&l);
Mock::VerifyAndClear(&l2);
Mock::VerifyAndClear(&lr);

// Local1 calls go to the remote throttle, not to local2.
EXPECT_CALL(lr, update(12));
throttle_.set_fn(12, 1);
wait();
Mock::VerifyAndClear(&l);
Mock::VerifyAndClear(&l2);
Mock::VerifyAndClear(&lr);

EXPECT_EQ(2, trainNode_->query_consist_length());
EXPECT_EQ(1, trainNode2_->query_consist_length());

// =================
LOG(INFO, "switching throttle2 back but no listener.");
// Local second throttle reassigned to the same train but without listener.
b = invoke_flow(&throttle2, TractionThrottleCommands::ASSIGN_TRAIN,
TRAIN_NODE_ID, false);
ASSERT_EQ(0, b->data()->resultCode);

EXPECT_EQ(2, trainNode_->query_consist_length());
EXPECT_EQ(0, trainNode2_->query_consist_length());

// Local1 calls go to the remote throttle, not to local2.
EXPECT_CALL(lr, update(11));
throttle_.set_fn(11, 1);
wait();
Mock::VerifyAndClear(&l);
Mock::VerifyAndClear(&l2);
Mock::VerifyAndClear(&lr);

// Local2 goes to local1 and remote
EXPECT_CALL(l, update(23));
EXPECT_CALL(lr, update(23));
throttle2.set_fn(23, 1);
wait();
Mock::VerifyAndClear(&l);
Mock::VerifyAndClear(&l2);
Mock::VerifyAndClear(&lr);

// If the remote node calls, only local1 will get it.
EXPECT_CALL(l, update(22));
throttler.set_fn(22, 1);
wait();
Mock::VerifyAndClear(&l);
Mock::VerifyAndClear(&l2);
Mock::VerifyAndClear(&lr);

// If a third node calls, all local1 and remote will get it.
EXPECT_CALL(l, update(15));
EXPECT_CALL(lr, update(15));
send_packet(":X195EB330N07710100000f0001;"); // fn 15 = 1
wait();
Mock::VerifyAndClear(&l);
Mock::VerifyAndClear(&l2);
Mock::VerifyAndClear(&lr);

EXPECT_EQ(2, trainNode_->query_consist_length());

// Local first throttle reassigned to the second train.
b = invoke_flow(&throttle_, TractionThrottleCommands::ASSIGN_TRAIN,
TRAIN_NODE_ID + 1, false);
ASSERT_EQ(0, b->data()->resultCode);
// removed listener.
EXPECT_EQ(1, trainNode_->query_consist_length());
}

} // namespace openlcb
Loading

0 comments on commit 5c1f88a

Please sign in to comment.