Traction Modem #807
Traction Modem #807
Conversation
Ensures that bash exits with an error when lcov barfs on the data.
targets/cov/traction_modem/Makefile
Outdated
There was a problem hiding this comment.
Since you're adding a new subdirectory, please run make on the toplevel to add these files to all targets.
src/traction_modem/TractionModem.hxx
Outdated
| #include "openlcb/TrainInterface.hxx" | ||
| #include "utils/format_utils.hxx" | ||
|
|
||
| namespace tractionmodem |
There was a problem hiding this comment.
would you like to rename this namespace to traction_modem as well?
src/traction_modem/TractionModem.hxx
Outdated
| // which is the heap allocator, and that the heap allocator will | ||
| // allocate memory on (at least) the native machine size boundary. | ||
| // This is important in order to be able to cast payload_.data() | ||
| // to a Defs::Message type for easier decoding. |
There was a problem hiding this comment.
// Otherwise if the architecture supports unaligned 32-bit reads, then we are fine for all allocators.
src/traction_modem/TractionModem.hxx
Outdated
| crc3_crc16_ccitt( | ||
| payload_.data() + sizeof(uint32_t), | ||
| payload_.size() - (sizeof(uint32_t) + sizeof(crc_calc)), | ||
| crc_calc.crc); |
There was a problem hiding this comment.
It is confusing that this does not need an & like &crc_calc.crc.
I understand why it doesn't, but it shows that the Defs::CRC is not a well encapsulated type.
There was a problem hiding this comment.
I'm not sure I agree with the statement "Defs::CRC is not a well encapsulated type". However, I changed the way I pass this in to make it a bit more clear.
| class MockPacketFlowInterface : public TestPacketFlowInterface | ||
| { | ||
| public: | ||
| MOCK_METHOD2(send, void(std::string &, unsigned)); |
There was a problem hiding this comment.
this doesn't need to be a separate class, but it can go straight into the TestPacketFlowInterface
There was a problem hiding this comment.
Ah, yes. Good call. Fixed.
|
|
||
| // Wait for exit. | ||
| wait_for_main_executor(); | ||
| while (!g_executor.active_timers()->empty()) |
There was a problem hiding this comment.
Since this code exists in async_if_test_helper.hxx already (called twait()), it would make sense to move it to test_main.hxx and call it wait_for_main_timers().
| wait_for_main_executor(); | ||
| testing::Mock::VerifyAndClearExpectations(&mPFI_); | ||
|
|
||
| // Send a message in pieces in order to exercise some of the other paths. |
There was a problem hiding this comment.
We should add another test where we send two packets in one ::write() command and expect both of those to arrive.
| @@ -116,6 +116,18 @@ void wait_for_main_executor() | |||
| guard->wait_for_notification(); | |||
| } | |||
|
|
|||
| /** Delays the current thread until all asynchronous processing and all | |||
| * pending timers have completed. */ | |||
| void wait_for_main_timers() | |||
There was a problem hiding this comment.
please also make twait() in async_if_test_helper.hxx call this function
src/traction_modem/TractionModem.hxx
Outdated
| @@ -358,6 +390,11 @@ private: | |||
| // allocation. | |||
| memmove(&payload_[0], &payload_[idx], recvCnt_ - idx); | |||
| recvCnt_ -= idx; | |||
| size_t new_size = | |||
| recvCnt_ < MIN_MESSAGE_SIZE ? MIN_MESSAGE_SIZE : recvCnt_; | |||
| payload_.resize(new_size); | |||
There was a problem hiding this comment.
Why do we resize here? The comment just above says that we are purposefully not resizing.
There was a problem hiding this comment.
You are correct. This is not needed. I had already made the necessary improvements elsewhere so that this is not needed. Removed.
| @@ -364,10 +364,12 @@ struct Defs | |||
| /// @param p wire formatted payload | |||
There was a problem hiding this comment.
The assumption is that this is a valid (correctly formatted) message.
There was a problem hiding this comment.
Yes, I improved the comment and...
- removed the unused preamble_prepended parameter, which is not needed
- added a "length" parameter for efficiency because it is already been extracted by the caller anyways
src/traction_modem/TractionModem.hxx
Outdated
| } | ||
|
|
||
| /// This is the memory space we will be using on the decoder. | ||
| uint8_t proxySpace_ = openlcb::MemoryConfigDefs::SPACE_DCC_CV; |
Adds a modem protocol implementation.