Memory Config Timeout Support (Datagrams) #809

bakerstu · 2025-01-16T23:31:30Z

Added support on the server side.
Added support on the client side.
Added unit test coverage.

… timeouts.

atanisoft

Looks good, will do some testing over this weekend.

@bobjacobsen can you confirm if JMRI is similarly handling timeout hints on datagram responses so I can test through that as well?

src/openlcb/MemoryConfig.hxx

src/openlcb/MemoryConfigClient.hxx

bobjacobsen · 2025-01-17T14:58:50Z

can you confirm if JMRI is similarly handling timeout hints on datagram responses so I can test through that as well?

@atanisoft I'm away from my setup at the moment, but I don't think JMRI (OpenLCB_Java) is doing this correctly. Will check, but it might be a day or two. If not, that needs to be fixed. There are some other things in the handling of the memory configuration protocol that need fixing too, so I'll try to do them all at the same time.

atanisoft · 2025-01-19T15:21:54Z

With the latest code I'm seeing the following warning/deprecation notice:

src/openlcb/MemoryConfig.hxx:1011:43: warning: bitwise operation between different enumeration types ‘openlcb::DatagramClient::ResponseFlag’ and ‘openlcb::DatagramDefs::Flag’ is deprecated [-Wdeprecated-enum-enum-conversion]
 1011 |             DatagramClient::REPLY_PENDING | space->get_write_timeout());

Not sure how best we should handle this other than using casting to an integer and then back to ResponseFlag.

bakerstu · 2025-01-20T00:06:17Z

With the latest code I'm seeing the following warning/deprecation notice:
src/openlcb/MemoryConfig.hxx:1011:43: warning: bitwise operation between different enumeration types ‘openlcb::DatagramClient::ResponseFlag’ and ‘openlcb::DatagramDefs::Flag’ is deprecated [-Wdeprecated-enum-enum-conversion]
 1011 |             DatagramClient::REPLY_PENDING | space->get_write_timeout());
Not sure how best we should handle this other than using casting to an integer and then back to ResponseFlag.

@atanisoft This is a good find. We should fix this. I can think of three possible solutions:

Static cast the operation.
Create a custom operator override.
Rework the enumerated types to be a bit more consistent and compatible, maybe even built off each other a bit. Possibly consider structs with bitfields.
1. This would still likely require some custom operator overrides.
2. One problem with bitfields is that there is no standard bit ordering enforced by the C/C++ standards. It happens that pretty much everyone uses the same order though, so maybe this is not so much an issue.

I very much would like to cleanup these enumerated types. I think they are not very well laid out, and could have been better designed. That said, I think it is too much of a change at this point. Therefore, I prefer solution 1. It is brute force, but at least self documenting in what the operation is doing.

WDYT?

atanisoft · 2025-01-20T00:33:34Z

I'm fine with opting for static_cast for now and seeing what we can do outside this PR as a better fit. I know C++20 has a bit better handling of enums, which may rely on extending standard variable types like uint8_t or similar.

"src/openlcb/MemoryConfig.hxx:1011:43: warning: bitwise operation between different enumeration types ‘openlcb::DatagramClient::ResponseFlag’ and ‘openlcb::DatagramDefs::Flag’ is deprecated [-Wdeprecated-enum-enum-conversion] 1011 | DatagramClient::REPLY_PENDING | space->get_write_timeout());"

bakerstu · 2025-01-25T16:16:12Z

I added the static_cast for both read and write timeout cases.

balazsracz · 2025-01-26T08:54:20Z

src/openlcb/DatagramImpl.hxx

 *
- * CANbus datagram parser and renderer flows.
+ * Datagram parser and renderer flows.


// These are independent of the transport method. For transport-specific renderers, see DatagranCan.cxx and Datagram Tcp.cxx.

balazsracz · 2025-01-26T08:57:39Z

src/openlcb/MemoryConfig.hxx

@@ -84,6 +84,21 @@ public:
    {
        return true;
    }
+
+    /// Get the read timeout. Default is no timeout.
+    /// @return The read timeout to reply with.


// , for example DatagramDefs::TIMEOUT_16 for 16 seconds.

balazsracz · 2025-01-26T08:58:27Z

src/openlcb/MemoryConfig.hxx

-     * MemorySpace::ERROR_AGAIN, and calls the Notifiable @param again when a
-     * re-try makes sense. The caller should call write once more, with the
-     * offset adjusted with the previously returned bytes. */
+    /// Write data to the address space.


// Called by the memory config service for incoming write requests.

balazsracz · 2025-01-26T09:04:04Z

src/openlcb/MemoryConfig.hxx

+    /// @param destination memory space offset address to write to
+    /// @param data data to write
+    /// @param len length of write data in bytes
+    /// @param error Error code that can be passed back. if != 0, the operation


I think this is a case where it is language-wise unclear whether an imperative statement applies to the caller or the callee. For this reason there is a rule in the Google style guide that function comments always have to be phrased in third person singular, which makes it clear that the description is about the function's implementation (i.e., the callee).

// @param error is the output argument for the error code. If the operation succeeded, // sets *error to zero. If the operation failed, sets *error to non-zero. If the operation needs // to be continued, then sets error to ERROR_AGAIN, and later calls the Notifiable when ready // to continue.

balazsracz · 2025-01-26T09:08:18Z

src/openlcb/MemoryConfig.hxx

+    /// @param error Error code that can be passed back. if != 0, the operation
+    ///        has failed. If the operation needs to be continued, then set
+    ///        error to ERROR_AGAIN, and later call the Notifiable to continue.
+    /// @param again notify() when the caller should call the write again, with


@param again Used only when *error was set to ERROR_AGAIN. Calls again.notify() when the operation is ready to be continued. The caller should then call the write again, with the offset (source) adjusted with the previously returned bytes.

balazsracz · 2025-01-26T10:32:44Z

src/openlcb/MemoryConfigClient.cxxtest

+    EXPECT_CALL(
+        tSpace_, write(0, _, test_payload.size(), _, NotifyCapture(&done)))
+            .WillOnce(DoAll(
+                SetArgPointee<3>(0x3FFF), Return(test_payload.size())));


I think this should be Return(0) since you are not actually accepting the payload

balazsracz · 2025-01-26T10:33:38Z

src/openlcb/MemoryConfigClient.cxxtest

+    clock.advance(MSEC_TO_NSEC(3200));
+    wait();
+    EXPECT_EQ(Defs::OPENMRN_TIMEOUT, b->data()->resultCode);
+    EXPECT_TRUE(


EXPECT_NE(0, memcmp())

balazsracz · 2025-01-26T10:34:31Z

src/openlcb/MemoryConfigClient.cxxtest

+    testing::Mock::VerifyAndClearExpectations(&tSpace_);
+
+    //
+    // Pass back timeout "32 seconds". Wait 31 seconds to reply.


this replies inline instead of after 31 seconds. You would need to capture done, wait 31 seconds, then call done notifiable.

This would need two expected calls on write(...) with the first one returning 0.

balazsracz · 2025-01-26T10:37:06Z

src/openlcb/MemoryConfigClient.cxxtest

+    EXPECT_CALL(
+        tSpace_, write(0, _, test_payload.size(), _, NotifyCapture(&done)))
+            .WillOnce(DoAll(
+                SetArgPointee<3>(0x3FFF), Return(test_payload.size())));


balazsracz · 2025-01-26T10:38:11Z

src/openlcb/MemoryConfigClient.hxx

@@ -402,7 +405,8 @@ private:

    Action read_response_timeout()
    {
-        if (responseCode_ & DatagramClient::OPERATION_PENDING)
+        if (responseCode_ & DatagramClient::OPERATION_PENDING ||
+            !timer_.is_triggered())


if the read finishes upon the first attempt, the timer is never started.
If this code is really needed, then we have to call timer_.set_triggered() on that path.

bakerstu added 4 commits January 16, 2025 16:49

Add server support for Datagram based memory configuration read/write…

0b980fb

… timeouts.

Add test coverage for read/write TIMEOUT_NONE.

e7b12a2

Add test coverage for read/write != TIMEOUT_NONE.

0a08238

Roll back indentation mistake.

55b9385

bakerstu requested a review from atanisoft January 16, 2025 23:31

Add Memory configuration Client support for read/write timeout flags.

3f1a3a2

atanisoft approved these changes Jan 17, 2025

View reviewed changes

src/openlcb/MemoryConfig.hxx Outdated Show resolved Hide resolved

src/openlcb/MemoryConfig.hxx Outdated Show resolved Hide resolved

src/openlcb/MemoryConfig.hxx Outdated Show resolved Hide resolved

src/openlcb/MemoryConfigClient.hxx Outdated Show resolved Hide resolved

atanisoft reviewed Jan 17, 2025

View reviewed changes

src/openlcb/MemoryConfigClient.hxx Outdated Show resolved Hide resolved

bakerstu added 5 commits January 17, 2025 17:10

Address review comments.

d064071

Fix math bug in time conversion.

2a2f19b

Properly handle a timeout on Memory Configuration server reply.

427a96b

Add proper Memory Configuration server timeout tests.

7fce9f7

Fix some incorrect comments.

bc67f2b

bakerstu changed the title ~~[WIP] Memory Config Timeout Support (Datagrams)~~ Memory Config Timeout Support (Datagrams) Jan 18, 2025

bakerstu requested a review from balazsracz January 18, 2025 20:29

balazsracz reviewed Jan 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory Config Timeout Support (Datagrams) #809

Memory Config Timeout Support (Datagrams) #809

bakerstu commented Jan 16, 2025 •

edited

Loading

atanisoft left a comment

bobjacobsen commented Jan 17, 2025 •

edited

Loading

atanisoft commented Jan 19, 2025

bakerstu commented Jan 20, 2025

atanisoft commented Jan 20, 2025 via email

bakerstu commented Jan 25, 2025

balazsracz Jan 26, 2025

balazsracz Jan 26, 2025

balazsracz Jan 26, 2025

balazsracz Jan 26, 2025

balazsracz Jan 26, 2025

balazsracz Jan 26, 2025

balazsracz Jan 26, 2025

balazsracz Jan 26, 2025

balazsracz Jan 26, 2025

balazsracz Jan 26, 2025

Memory Config Timeout Support (Datagrams) #809

Are you sure you want to change the base?

Memory Config Timeout Support (Datagrams) #809

Conversation

bakerstu commented Jan 16, 2025 • edited Loading

atanisoft left a comment

Choose a reason for hiding this comment

bobjacobsen commented Jan 17, 2025 • edited Loading

atanisoft commented Jan 19, 2025

bakerstu commented Jan 20, 2025

atanisoft commented Jan 20, 2025 via email

bakerstu commented Jan 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakerstu commented Jan 16, 2025 •

edited

Loading

bobjacobsen commented Jan 17, 2025 •

edited

Loading