uniba-swt / libbidib Goto Github PK

When a node sends a command, and the recipient of the command is expected to reply with a message of a certain type ("response type"), we save that a reply of a certain type addressed to the sending node is to be expected. This "saving" happens by adding an entry into the so-called "response queue". A response queue entry holds the response-type to be expected, the time it was created, and the action id of the sent command.

An example for how this is shown in the log can be seen below:

libbidib: Send to: 0x00 0x00 0x00 0x00 seq: 47 type: MSG_CS_DRIVE (0x64) action id: 242
libbidib: Message bytes: 0x0c 0x00 0x2f 0x64 0x06 0x00 0x03 0x01 0x80 0x00 0x00 0x00 0x00
libbidib: Received packet
libbidib: Received from: 0x00 0x00 0x00 0x00 seq: 104 type: MSG_CS_DRIVE_ACK (0xe2) action id: 242
libbidib: Message bytes: 0x06 0x00 0x68 0xe2 0x06 0x00 0x01
libbidib: Feedback for action id 242: Train: cargo_green acknowledgement level: 1

A message of type MSG_CS_DRIVE is sent, its action id is 242. Later, a message of type MSG_CS_DRIVE_ACK is received, which matches the expected response type. It is thus traced to be related to action id 242, which is logged accordingly. In the operation of matching the received response against the expected response, i.e. the head of the response queue, the first entry of the response queue is consumed.

The functionality/strategy outlined above works great for commands that result in at most one expected reply. However, there are commands which may result in one or two responses. For example: The command to set a point (an accessory) to a certain state may cause one or two replies of type MSG_ACCESSORY_STATE. This can be seen in the shortened log below:

libbidib: Switch point: point4 on board: onecontrol3 (0x0c 0x00 0x00 0x00) to aspect: normal (0x01) with action id: 243
libbidib: Send to: 0x0c 0x00 0x00 0x00 seq: 10 type: MSG_ACCESSORY_SET (0x38) action id: 243
[...]
libbidib: Received packet
libbidib: Received from: 0x0c 0x00 0x00 0x00 seq: 66 type: MSG_ACCESSORY_STATE (0xb8) action id: 243
libbidib: Message bytes: 0x09 0x0c 0x00 0x42 0xb8 0x01 0x01 0x02 0x01 0x03
libbidib: Feedback for action id 243: Point accessory: point4 execution: normal not reached verified with wait time: 0.3s
[...]
libbidib: Received packet
libbidib: Received from: 0x0c 0x00 0x00 0x00 seq: 70 type: MSG_ACCESSORY_STATE (0xb8) action id: 0
libbidib: Message bytes: 0x09 0x0c 0x00 0x46 0xb8 0x01 0x01 0x02 0x00 0x00
libbidib: Feedback for action id 0: Point accessory: point4 execution: normal reached verified with wait time: 0.0s

The command (message type MSG_ACCESSORY_SET) is to set the accessory "point4" to the aspect "normal", its action id is 243. A response queue entry is created, the expected response type is MSG_ACCESSORY_STATE.
The first reply of type MSG_ACCESSORY_STATE matches the entry in the response queue and is thus logged to be related to action id 243. The message content tells us that point4 has not yet reached aspect "normal", and that the remaining time to wait for the accessory to reach the target aspect is 0.3 seconds. The corresponding response queue entry is removed.
0.3s later, the second message of type MSG_ACCESSORY_STATE is received, telling us that point4 has now reached the target aspect "normal". Receiving the first message of this type lead to the removal of the response queue entry. Therefore, this message does not match any entry in the response queue. It can't be related to a non-zero action id and is thus printed with action id 0.
Note that a MSG_ACCESSORY_SET can also result in just one reply. For example, when setting a signal to a state, only one response of type MSG_ACCESSORY_STATE will be received.

The functionality for relating responses to commands and their action id should be able to correctly relate action ids with more than one expected response.

Thoughts:
Part of what makes this tricky is that the current implementation works with a queue and only compares received messages against the head of said queue. This works based on the assumption that responses are received in the same order as the commands were sent.
As there may be a considerable delay between the first and second MSG_ACCESSORY_STATE, it is not an option to just put in two response queue entries in succession to catch these. A different command with its own expected response may be sent between the first and second response from the accessory, and receive its response before the second accessory response arrives. In that case, the response to the "new" command wouldn't match the head of the response queue and be discarded.

In some malloc calls destined for strings, the size is calculated as sizeof(char) * len + 1. Example.
In most of bidib, its sizeof(char) * (len + 1). Ideally, should be consistent in all occurences, even if the resulting values don't change as sizeof(char) is likely always 1.

The track coverage test case of libbidib/test/physical-test is incomplete for the SWTbahn Standard platform. For example, point 7 is only tested in the normal position.

• Points are driven through in all positions and in all directions

The libbidib/test/physical-test/Readme.md file is incomplete:

• Under the main heading, add a brief description of the purpose and capabilities of the test suite
• Mention any restrictions on the possible SWTbahn platforms
• Mention any limitations of the test cases
• Add a brief description to each test case
• Elaborate on the dependencies
• Elaborate on the build procedure
• Mention how to stop a test case
• Elaborate on the importance of the list of points and signals

It would be nice if the CMakeLists.txt configured an install target (to move the built libs to e.g. /usr/local/lib on Linux), as well as a package-config file to point to the installed libraries and the include directory.

Moreover, it would be nice if there was a CMake flag for disabling/enabling compilation of the test targets.

I'll create a prototype version and then open a PR.

Hello, I was looking at this commit d2ed8ca#diff-50d858e0985ecc7f60418aaf0cc5ab587f42c2570a884095a9e8ccacd0f6545c and was wondering, what is the reason this was renamed? A loco is more likely to be recognized and addressed by it's driving direction than it's left/right orientation.

4.1.2. Uplink: System messages

• MSG_SYS_ERROR
• BIDIB_ERR_BUS: Short-circuit is value 0

4.7.5. Features for bidirectional detectors

• FEATURE_BM_ISTSPEED_AVAILABLE
• FEATURE_BM_ISTSPEED_INTERVAL

4.7.7. Uplink: Messages for bidirectional detectors

• MSG_BM_SPEED

Handling of error messages in libbidib

• Processing of MSG_SYS_ERROR

Handling of speed messages in libbidib

• Detection of MSG_BM_SPEED
• Processing of MSG_BM_SPEED

@BLuedtke Should we commit the pthread_rwlock_rdlock changes from the stop-delay-benchmarking branch to the master branch?

The state of a Kehrschleifenmodul (reversing loop) is stored on the GBMboost board that it is connected to. We can read the state via the Configuration Value (CV) 51 that is local of the GBM16T it is connected to.

1. Define the CV of each Kehrschleifenmodule as part of the track configuration file.

boards:
  - id: master
    reversers:
      - id: reverser1
        cv: 10051
      - id: reverser2
        cv: 20051

2. Update the track parser to create a new state object for reversers.

3. Define high-level functions for updating the state of a reverser from a GBM16T into the library.

// Working prototype:
t_bidib_node_address_query nodeaddr = bidib_get_nodeaddr("master");
bidib_send_vendor_get(nodeaddr.address, (uint8_t)5, (uint8_t *)&"20051", 0);

// Possible libbidib API:
void bidib_update_reverser_state(char *reverser_name) {
  uint8_t *reverser = bidib_get_reverser(reverser_name);
  bidib_send_vendor_get(reverser->nodeaddr, reverser->cv_length, reverser->cv, 0);
}

Result of bidib_send_vendor_get(0.0.0, 5, "20051"), which asks the for the value of CV 20051 (current state of the Kehrschleifenmodul) from node address 0.0.0 (GBMboost master):

libbidib: Send to: 0x00 0x00 0x00 0x00 seq: 38 type: MSG_VENDOR_GET (0x17) action id: 0
libbidib: Message bytes: 0x09 0x00 0x26 0x17   0x05 0x32 0x30 0x30 0x35 0x31

Response from GBMboost master:

libbidib: Received from: 0x00 0x00 0x00 0x00 seq: 38 type: MSG_VENDOR (0x93) action id: 0
libbidib: Message bytes: 0x0b 0x00 0x26 0x93   0x05 0x32 0x30 0x30 0x35 0x31   0x01 0x31

The value of CV 20051 is "1" (ASCII value 0x31).

Thus, bidib_transmission_receive.c needs to look for MSG_VENDOR and to check whether the CV matches a reverser. If so, the value is saved into the state.

4. Define high-level functions for querying the state of a reverser. The state should be marked as stale (-1) to indicate that an update is necessary before it can be queried again.

When placing a train on the tracks with right orientation, we get a libbidib error message that looks as follows:
libbidib: No train configured for dcc address 0x01 0x80
Driving the train (i.e. setting a speed) still works as expected.

When placing the same train with left orientation, this does not occur.

When sending a message via a node, first bidib determines if the node is ready to send.
This involves, among other things, checking whether the node is stalled, or, whether a supernode of the node is stalled. The stall check is performed in a function here.

If the node or a supernode is stalled, the linked function returns false. A node is stalled if the member "stall" is true and the address_stack (input param to linked fct) is NOT contained in the member stall_affected_nodes_queue.
Thus, the linked fct attempts to find the addr_stack in the queue stall_affected_nodes_queue by using g_queue_find, see line 114. As the addr_stack is an array of 4 uint8_t's, a custom comparison function is used, as the comparison shall be element-wise, not a pointer comparison. This comparator is called bidib_node_stall_queue_entry_equals (see line 99ff.).
The call to g_queue_find looks as follows: !g_queue_find(state->stall_affected_nodes_queue, bidib_node_stall_queue_entry_equals).
Note that the element to find is not actually given here! This code just always returns true, the comparator is never called. g_queue_find is supposed to be called with the queue and the element to find (see here). To use a custom comparator, we need to use g_queue_find_custom (see here). It shall be called like this: !g_queue_find_custom(state->stall_affected_nodes_queue, addr_stack, (GCompareFunc)bidib_node_stall_queue_entry_equals).

The unit tests related to this part (bidib_send_tests) pass as the "stall" member is apparently sufficient to indicate whether the node is stalled or not. However, there's probably a reason for the existence of the stall_affected_nodes_queue, so this comparison call should be fixed.

With Commit f6f11a2, I attempted to fix a bug in how signal-dcc parameters are calculated. I based the bugfix on the code for dcc points. However, I made a mistake doing that.

On line 256 in bidib_highlevel_setter, the .value member should be left-shifted by 5, note ORed with a 1 that was left-shifted by 5. Compare the following:

1. Current buggy signals dcc assignment:
   

2. Correct points dcc assignment:
   

The necessary action to be taken is obvious.

The recently added install target (see PR) works. There is no way to automatically uninstall what can be installed though, which is inconvenient. I'll work on adding an appropriate uninstall option via cmake/make.

In Section 4.7.7 of the BiDiB standard, the address format of locomotives is described. An address has a low and high part, called addrl and addrh respectively.

What is interesting is that bits 7 and 6 of addrh (assuming 0 indexing) provides the locomotive's orientation relative to the track. 00 means that the locomotive faces one way, while 10 means it faces the other.

Could this orientation information be included in the train state?

One of the places where locomotive orientation is updated?

Trains not physically on the tracks are sometimes reported by the hardware as being detected, e.g., by MSG_BM_SPEED and MSG_BM_ADDRESS.