-
Notifications
You must be signed in to change notification settings - Fork 12
Test double buffer accessor ns #551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,338 @@ | ||
| #define BOOST_TEST_MODULE testDoubleBufferAccessor | ||
| #include "Device.h" | ||
| #include "DoubleBufferAccessor.h" | ||
| #include "DummyBackend.h" | ||
|
|
||
| #include <boost/test/unit_test.hpp> | ||
|
|
||
| using namespace ChimeraTK; | ||
|
|
||
| namespace { | ||
|
|
||
| /* expose protected members */ | ||
| template<typename T> | ||
| class TestableDoubleBufferAccessor : public DoubleBufferAccessor<T> { | ||
| public: | ||
| using DoubleBufferAccessor<T>::DoubleBufferAccessor; | ||
| using DoubleBufferAccessor<T>::buffer_2D; | ||
| }; | ||
| } // namespace | ||
|
|
||
| BOOST_AUTO_TEST_CASE(test_current_buffer_selection) { | ||
| Device device; | ||
| device.open("(dummy?map=simpleJsonFile.jmap)"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should get our own jmap file. The "simple" now contains everything, which is very crowded and difficult to understand. This is an additional hurdle when trying to debug stuff. A stripped down map file makes this easier. |
||
|
|
||
| auto backend = boost::dynamic_pointer_cast<NumericAddressedBackend>(device.getBackend()); | ||
| BOOST_REQUIRE(backend); | ||
|
|
||
| auto mutex = std::make_shared<detail::CountedRecursiveMutex>(); | ||
|
|
||
| auto registerInfo = backend->getRegisterInfo("DAQ.FD"); | ||
| BOOST_REQUIRE(registerInfo.doubleBuffer != std::nullopt); | ||
|
|
||
| auto dbInfo = registerInfo.doubleBuffer.value(); | ||
|
|
||
| TestableDoubleBufferAccessor<int> accessor( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just get the accessor through the regular device interface. Use it as normal code would.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW. This call is wrong. It needs the mutex that is shared by the backend. You can't just give "any" mutex. |
||
| dbInfo, backend, mutex, RegisterPath("/DAQ/FD"), 16384, 0, AccessModeFlags{}); | ||
|
|
||
| /* firmware-visible buffers */ | ||
| auto buf0 = device.getTwoDRegisterAccessor<int>("/DAQ/FD/BUF0"); | ||
| auto buf1 = device.getTwoDRegisterAccessor<int>("/DAQ/FD/BUF1"); | ||
|
|
||
| auto inactive = device.getOneDRegisterAccessor<int>("/DAQ/DOUBLE_BUF/INACTIVE_BUF_ID"); | ||
|
|
||
| /* simulate firmware writing to buffer0 */ | ||
| buf0[0][0] = 4; | ||
| buf0[0][1] = 8; | ||
| buf0[0][2] = 12; | ||
| buf0[0][3] = 16; | ||
| buf0.write(); | ||
|
|
||
| /* firmware state: | ||
| inactive = 1 → firmware writes BUF0 | ||
| */ | ||
|
|
||
| inactive[0] = 1; | ||
| inactive.write(); | ||
|
|
||
| accessor.doPreRead(TransferType::read); | ||
| accessor.doReadTransferSynchronously(); | ||
| accessor.doPostRead(TransferType::read, true); | ||
|
Comment on lines
+58
to
+60
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you call these individually? It should just be |
||
|
|
||
| BOOST_CHECK_EQUAL(accessor.buffer_2D[0][0], 4); | ||
| BOOST_CHECK_EQUAL(accessor.buffer_2D[0][1], 8); | ||
| BOOST_CHECK_EQUAL(accessor.buffer_2D[0][2], 12); | ||
| BOOST_CHECK_EQUAL(accessor.buffer_2D[0][3], 16); | ||
|
|
||
| inactive[0] = 0; | ||
| inactive.write(); | ||
|
|
||
| buf1[0][0] = 140; | ||
| buf1[0][1] = 144; | ||
| buf1[0][2] = 148; | ||
| buf1[0][3] = 152; | ||
| buf1.write(); | ||
|
|
||
| accessor.doPreRead(TransferType::read); | ||
| accessor.doReadTransferSynchronously(); | ||
| accessor.doPostRead(TransferType::read, true); | ||
|
Comment on lines
+76
to
+78
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you call these individually? It should just be |
||
|
|
||
| BOOST_CHECK_EQUAL(accessor.buffer_2D[0][0], 140); | ||
| BOOST_CHECK_EQUAL(accessor.buffer_2D[0][1], 144); | ||
| BOOST_CHECK_EQUAL(accessor.buffer_2D[0][2], 148); | ||
| BOOST_CHECK_EQUAL(accessor.buffer_2D[0][3], 152); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(test_transfer_lock_blocks_other_accessor) { | ||
| Device device; | ||
| device.open("(dummy?map=simpleJsonFile.jmap)"); | ||
|
|
||
| auto backend = boost::dynamic_pointer_cast<NumericAddressedBackend>(device.getBackend()); | ||
| BOOST_REQUIRE(backend); | ||
|
|
||
| auto mutex = std::make_shared<detail::CountedRecursiveMutex>(); | ||
|
|
||
| auto registerInfo = backend->getRegisterInfo("DAQ.FD"); | ||
| auto dbInfo = registerInfo.doubleBuffer.value(); | ||
|
|
||
| TestableDoubleBufferAccessor<int> accessor1( | ||
| dbInfo, backend, mutex, RegisterPath("/DAQ/FD"), 16384, 0, AccessModeFlags{}); | ||
|
|
||
| TestableDoubleBufferAccessor<int> accessor2( | ||
| dbInfo, backend, mutex, RegisterPath("/DAQ/FD"), 16384, 0, AccessModeFlags{}); | ||
|
|
||
| std::atomic<bool> secondEntered{false}; | ||
|
|
||
| /* Thread 1 acquires transfer lock */ | ||
| accessor1.doPreRead(TransferType::read); | ||
|
|
||
| /* start accessor2 asynchronously */ | ||
| auto future = std::async(std::launch::async, [&] { | ||
| accessor2.doPreRead(TransferType::read); | ||
| accessor2.doPostRead(TransferType::read, false); | ||
| }); | ||
|
|
||
| /* accessor2 must still be blocked */ | ||
| auto status = future.wait_for(std::chrono::milliseconds(50)); | ||
| BOOST_CHECK(status == std::future_status::timeout); | ||
|
|
||
| /* release lock */ | ||
| accessor1.doPostRead(TransferType::read, false); | ||
|
|
||
|
Comment on lines
+107
to
+121
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, you are testing at the internal interface. And you are not even calling the transfer itself. Do you remember why we did the locking in preRead()? I know we discussed it, but I forgot. |
||
| /* now accessor2 must complete */ | ||
| status = future.wait_for(std::chrono::milliseconds(200)); | ||
| BOOST_CHECK(status == std::future_status::ready); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(test_mutex_usecount) { | ||
| auto mutex = std::make_shared<detail::CountedRecursiveMutex>(); | ||
|
|
||
| mutex->lock(); | ||
| BOOST_CHECK_EQUAL(mutex->useCount(), 1); | ||
|
|
||
| mutex->lock(); | ||
| BOOST_CHECK_EQUAL(mutex->useCount(), 2); | ||
|
|
||
| mutex->unlock(); | ||
| BOOST_CHECK_EQUAL(mutex->useCount(), 1); | ||
|
|
||
| mutex->unlock(); | ||
| BOOST_CHECK_EQUAL(mutex->useCount(), 0); | ||
| } | ||
|
Comment on lines
+127
to
+141
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. This test is testing the CountedRecursiveMutex. That's not the scope here. Just remove. |
||
|
|
||
| BOOST_AUTO_TEST_CASE(test_exception_does_not_leave_lock) { | ||
| Device device; | ||
| device.open("(dummy?map=simpleJsonFile.jmap)"); | ||
|
|
||
| auto backend = boost::dynamic_pointer_cast<NumericAddressedBackend>(device.getBackend()); | ||
| BOOST_REQUIRE(backend); | ||
|
|
||
| auto mutex = std::make_shared<detail::CountedRecursiveMutex>(); | ||
|
|
||
| auto registerInfo = backend->getRegisterInfo("DAQ.FD"); | ||
| auto dbInfo = registerInfo.doubleBuffer.value(); | ||
|
|
||
| TestableDoubleBufferAccessor<int> accessor( | ||
| dbInfo, backend, mutex, RegisterPath("/DAQ/FD"), 16384, 0, AccessModeFlags{}); | ||
|
|
||
| // Acquire lock via pre-read | ||
| accessor.doPreRead(TransferType::read); | ||
| BOOST_CHECK_EQUAL(mutex->useCount(), 1); // lock acquired | ||
|
|
||
| // Simulate exception during transfer | ||
| bool exceptionThrown = false; | ||
| try { | ||
| throw std::runtime_error("simulated transfer exception"); | ||
| } | ||
| catch(const std::runtime_error&) { | ||
| exceptionThrown = true; | ||
| // mandate: still call postRead to release lock | ||
| accessor.doPostRead(TransferType::read, false); | ||
| } | ||
|
|
||
| BOOST_CHECK(exceptionThrown); | ||
| BOOST_CHECK_EQUAL(mutex->useCount(), 0); // lock released | ||
| } | ||
|
|
||
|
Comment on lines
+143
to
+176
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test does not test anything. The accessor does not know about the exception at all. The correct test would be: Use the exception backend to raise an exception during transfer. Then check that the next transfer in another thread can be executed (no deadlock). This is observable behaviour from the outside, that's all that counts. But don't implement it here. It is all covered by the UnifiedBackendTest. |
||
| // ------------------------------------------------------------ | ||
| // Test that _enableDoubleBufferReg toggles during pre/post read | ||
| BOOST_AUTO_TEST_CASE(test_firmware_handshake_toggle) { | ||
| Device device; | ||
| device.open("(dummy?map=simpleJsonFile.jmap)"); | ||
| auto backend = boost::dynamic_pointer_cast<NumericAddressedBackend>(device.getBackend()); | ||
| auto mutex = std::make_shared<detail::CountedRecursiveMutex>(); | ||
| auto registerInfo = backend->getRegisterInfo("DAQ.FD"); | ||
| auto dbInfo = registerInfo.doubleBuffer.value(); | ||
|
|
||
| TestableDoubleBufferAccessor<int> accessor( | ||
| dbInfo, backend, mutex, RegisterPath("/DAQ/FD"), 16384, 0, AccessModeFlags{}); | ||
|
|
||
| // Get the actual register for handshake | ||
| auto enableReg = device.getOneDRegisterAccessor<uint32_t>("/DAQ/DOUBLE_BUF/ENA"); | ||
| enableReg.read(); | ||
| BOOST_CHECK_EQUAL(enableReg[0], 1); // must be enabled*/ | ||
|
|
||
| // Pre-read should disable the double buffer | ||
| accessor.doPreRead(TransferType::read); | ||
| enableReg.read(); | ||
| BOOST_CHECK_EQUAL(enableReg[0], 0); // must be disabled | ||
|
|
||
| // Read transfer | ||
| accessor.doReadTransferSynchronously(); | ||
|
|
||
| // Post-read should re-enable the double buffer | ||
| accessor.doPostRead(TransferType::read, true); | ||
| enableReg.read(); | ||
| BOOST_CHECK_EQUAL(enableReg[0], 1); // must be re-enabled*/ | ||
| } | ||
|
|
||
| // ------------------------------------------------------------ | ||
| // Test that hasNewData=false does not swap buffer_2D | ||
| BOOST_AUTO_TEST_CASE(test_has_new_data_false) { | ||
| Device device; | ||
| device.open("(dummy?map=simpleJsonFile.jmap)"); | ||
| auto backend = boost::dynamic_pointer_cast<NumericAddressedBackend>(device.getBackend()); | ||
| auto mutex = std::make_shared<detail::CountedRecursiveMutex>(); | ||
| auto registerInfo = backend->getRegisterInfo("DAQ.FD"); | ||
| auto dbInfo = registerInfo.doubleBuffer.value(); | ||
|
|
||
| TestableDoubleBufferAccessor<int> accessor(dbInfo, backend, mutex, RegisterPath("/DAQ/FD"), 4, 0, AccessModeFlags{}); | ||
|
|
||
| auto buf0 = device.getTwoDRegisterAccessor<int>("/DAQ/FD.BUF0"); | ||
| auto inactive = device.getOneDRegisterAccessor<int>("/DAQ/DOUBLE_BUF/INACTIVE_BUF_ID"); | ||
| inactive[0] = 1; | ||
| inactive.write(); | ||
|
|
||
| buf0[0] = {100, 200, 300, 400}; | ||
| buf0.write(); | ||
|
|
||
| // Initial read | ||
| accessor.doPreRead(TransferType::read); | ||
| accessor.doReadTransferSynchronously(); | ||
| accessor.doPostRead(TransferType::read, true); | ||
|
|
||
| auto oldBuffer0 = accessor.buffer_2D[0]; | ||
|
|
||
| // update reg | ||
| buf0[0] = {20, 40, 60, 80}; | ||
| buf0.write(); | ||
|
|
||
| // hasNewData=false → buffer_2D should not update | ||
| accessor.doPreRead(TransferType::read); | ||
| accessor.doReadTransferSynchronously(); | ||
| accessor.doPostRead(TransferType::read, false); | ||
|
|
||
| BOOST_CHECK_EQUAL_COLLECTIONS( | ||
| oldBuffer0.begin(), oldBuffer0.end(), accessor.buffer_2D[0].begin(), accessor.buffer_2D[0].end() // still old | ||
| ); | ||
| // hasNewData=true → buffer_2D should update | ||
| accessor.doPreRead(TransferType::read); | ||
| accessor.doReadTransferSynchronously(); | ||
| accessor.doPostRead(TransferType::read, true); | ||
|
|
||
| BOOST_CHECK_EQUAL_COLLECTIONS( | ||
| buf0[0].begin(), buf0[0].end(), accessor.buffer_2D[0].begin(), accessor.buffer_2D[0].end() // now updated | ||
| ); | ||
| } | ||
|
Comment on lines
+210
to
+256
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, you are testing deep down of the stuff where the framework is responsible. Calling doPostRead() with newData flag true and false is done by the TransferElement. This is all covered by the UnifiedBackendTest. No need to write dedicated tests here. |
||
| // ------------------------------------------------------------ | ||
| // Test multiple channels | ||
| BOOST_AUTO_TEST_CASE(test_multiple_channels) { | ||
| Device device; | ||
| device.open("(dummy?map=simpleJsonFile.jmap)"); | ||
| auto backend = boost::dynamic_pointer_cast<NumericAddressedBackend>(device.getBackend()); | ||
| auto mutex = std::make_shared<detail::CountedRecursiveMutex>(); | ||
| auto dbInfo = backend->getRegisterInfo("DAQ.FD").doubleBuffer.value(); | ||
|
|
||
| TestableDoubleBufferAccessor<int> accessor(dbInfo, backend, mutex, "/DAQ/FD", 4, 0, AccessModeFlags{}); | ||
|
|
||
| auto buf0 = device.getTwoDRegisterAccessor<int>("/DAQ/FD.BUF0"); | ||
| auto inactive = device.getOneDRegisterAccessor<int>("/DAQ/DOUBLE_BUF/INACTIVE_BUF_ID"); | ||
| inactive[0] = 1; | ||
| inactive.write(); | ||
|
|
||
| // simulate 2 channels | ||
| buf0[0] = {16, 20, 24, 32}; | ||
| buf0[1] = {100, 200, 300, 400}; | ||
| buf0.write(); | ||
|
|
||
| accessor.doPreRead(TransferType::read); | ||
| accessor.doReadTransferSynchronously(); | ||
| accessor.doPostRead(TransferType::read, true); | ||
|
|
||
| BOOST_CHECK_EQUAL_COLLECTIONS( | ||
| buf0[0].begin(), buf0[0].end(), accessor.buffer_2D[0].begin(), accessor.buffer_2D[0].end()); | ||
| BOOST_CHECK_EQUAL_COLLECTIONS( | ||
| buf0[1].begin(), buf0[1].end(), accessor.buffer_2D[1].begin(), accessor.buffer_2D[1].end()); | ||
| } | ||
|
Comment on lines
+258
to
+286
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this test that has anything to do with double buffering that is not tested above? |
||
|
|
||
| // ------------------------------------------------------------ | ||
| // Test mayReplaceOther logic | ||
| BOOST_AUTO_TEST_CASE(test_may_replace_other) { | ||
| Device device; | ||
| device.open("(dummy?map=simpleJsonFile.jmap)"); | ||
| auto backend = boost::dynamic_pointer_cast<NumericAddressedBackend>(device.getBackend()); | ||
| auto mutex = std::make_shared<detail::CountedRecursiveMutex>(); | ||
| auto registerInfo = backend->getRegisterInfo("DAQ.FD"); | ||
| auto dbInfo = registerInfo.doubleBuffer.value(); | ||
|
|
||
| auto accessor1 = boost::make_shared<TestableDoubleBufferAccessor<int>>( | ||
| dbInfo, backend, mutex, RegisterPath("/DAQ/FD"), 16384, 0, AccessModeFlags{}); | ||
|
|
||
| auto accessor2 = boost::make_shared<TestableDoubleBufferAccessor<int>>( | ||
| dbInfo, backend, mutex, RegisterPath("/DAQ/FD"), 16384, 0, AccessModeFlags{}); | ||
|
|
||
| // must not replace itself | ||
| BOOST_CHECK(!accessor1->mayReplaceOther(accessor1)); | ||
|
|
||
| BOOST_CHECK(accessor1->mayReplaceOther(accessor2)); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, this is covered by the UnifiedBackendTest. No need to write a dedicated test. |
||
|
|
||
| BOOST_AUTO_TEST_CASE(test_write_not_allowed) { | ||
| Device device; | ||
| device.open("(dummy?map=simpleJsonFile.jmap)"); | ||
| auto backend = boost::dynamic_pointer_cast<NumericAddressedBackend>(device.getBackend()); | ||
| auto mutex = std::make_shared<detail::CountedRecursiveMutex>(); | ||
| auto dbInfo = backend->getRegisterInfo("DAQ.FD").doubleBuffer.value(); | ||
|
|
||
| TestableDoubleBufferAccessor<int> accessor(dbInfo, backend, mutex, "/DAQ/FD", 4, 0, AccessModeFlags{}); | ||
|
|
||
| BOOST_CHECK_THROW(accessor.doPreWrite(TransferType::write, {}), ChimeraTK::logic_error); | ||
|
|
||
| // doPostWrite does nothing but should not throw | ||
| BOOST_CHECK_NO_THROW(accessor.doPostWrite(TransferType::write, {})); | ||
| } | ||
|
Comment on lines
+310
to
+323
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, this test is wrong. Again, you are testing to deep down. doPostWrite() might throw. That's not how it is specified. This part should be covered by the UnifiedBackendTest. The correct test would just be: |
||
|
|
||
| // ------------------------------------------------------------ | ||
| // Edge case: numberOfWords = 0 | ||
| BOOST_AUTO_TEST_CASE(test_zero_words) { | ||
| Device device; | ||
| device.open("(dummy?map=simpleJsonFile.jmap)"); | ||
| auto backend = boost::dynamic_pointer_cast<NumericAddressedBackend>(device.getBackend()); | ||
| auto mutex = std::make_shared<detail::CountedRecursiveMutex>(); | ||
| auto registerInfo = backend->getRegisterInfo("DAQ.FD"); | ||
| auto dbInfo = registerInfo.doubleBuffer.value(); | ||
|
|
||
| TestableDoubleBufferAccessor<int> accessor(dbInfo, backend, mutex, RegisterPath("/DAQ/FD"), 0, 0, AccessModeFlags{}); | ||
|
|
||
| BOOST_CHECK_EQUAL(accessor.getNumberOfSamples(), 0); | ||
| } | ||
|
Comment on lines
+326
to
+338
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not an edge cage but a wrong test. An Accessor with 0 elements should not exist, it should even have an assertion that it never is instantiated. Correct behaviour: If call I don't know if this is covered in the UnifiedBackendTest. If not, it should be added there (and in the specification if it is not there). Should be checked. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't. You should test observable behaviour, not implementation details. Internal interfaces might be subject to refactoring, which breaks the test.