Part1 of : Feat double buffer pushtype #534
Conversation
nlohmann still cannot do it for optional out of the box.
killenb
left a comment
killenb
left a comment
There was a problem hiding this comment.
Before this can go to the master we need a unit test (unified backend test) that is testing the new decorator.
The implementation should be rudimentary functional, but the register merging and behaviour in the transfer group has to be studied. I suspect this is always reading the primary and the secondary buffer in the transfer group and is only picking one of them. I might be wrong here.
| using ChimeraTK::NDRegisterAccessorDecorator<UserType>::buffer_2D; | ||
| using ChimeraTK::NDRegisterAccessorDecorator<UserType>::_target; | ||
| NumericDoubleBufferAccessorDecorator(const boost::shared_ptr<ChimeraTK::NDRegisterAccessor<UserType>>& target, | ||
| std::optional<NumericAddressedRegisterInfo::DoubleBufferInfo> doubleBufferConfig, |
There was a problem hiding this comment.
I am wondering what the plugin does if there is nothing in the optional. This should just be the DoubleBufferInfo
There was a problem hiding this comment.
The name should not be ND, that means n-dimensional (in contrast to oneD and twoD and scalar)
| namespace ChimeraTK { | ||
|
|
||
| template<typename UserType> | ||
| class NumericDoubleBufferAccessorDecorator : public NDRegisterAccessorDecorator<UserType> { |
There was a problem hiding this comment.
Do we want to name this "Numeric"? It is foreseen to also be used by the LMAP backend, eventually.
| NumericDoubleBufferAccessorDecorator(const boost::shared_ptr<ChimeraTK::NDRegisterAccessor<UserType>>& target, | ||
| std::optional<NumericAddressedRegisterInfo::DoubleBufferInfo> doubleBufferConfig, | ||
| const boost::shared_ptr<DeviceBackend>& backend, | ||
| std::shared_ptr<NumericAddressedBackend::DoubleBufferControlState> controlState); |
There was a problem hiding this comment.
I would move the DoubleBufferControlState into the DoubleBufferAccessorDecorator file. Like that it can also be used by the lmap backend, which should not depend on the NumericAddressedBackend.
| std::list<boost::shared_ptr<TransferElement>> getInternalElements() override { return {}; } | ||
|
|
||
| void replaceTransferElement(boost::shared_ptr<ChimeraTK::TransferElement> /* newElement */) override { | ||
| // do nothing, we do not support merging of DoubleBufferAccessorDecorators |
There was a problem hiding this comment.
We will have to support merging eventually because we don't want to do individual transfers of neighbouring registers. Once we can describe individual channels of multiplexed registers, we have to support it. We cannot transfer the whole DAQ area 16 times and only extract one channel.
But to be honest, I have no idea yet how to combine that with double buffering. Elements are merged when you put them into the transfer group, but we cannot put the registers of the promary and the secondary accessor into the same transfer group. So we probably need two "under the hood" transfer groups for each DoubleBufferControlState.
Now that I think about it, I don't even know if it works cleanly in the current implementation.
|
|
||
| struct DoubleBufferingInfo { | ||
| struct Address { | ||
| AddressType type{AddressType::DMA}; |
There was a problem hiding this comment.
Why is DMA the default? It can be anything.
| size_t bytesPerElement{0}; | ||
|
|
||
| struct DoubleBufferingInfo { | ||
| struct Address { |
There was a problem hiding this comment.
There is a struct Address below which is different. Something is not right here.
There was a problem hiding this comment.
Ah, this is in the DoubleBufferingInfo, the other is outside. Are they the same? Why not put the Address from below to the top and use it here. If it's different, then SecondaryAddress is a better name here.
| auto buf0Name = primaryName + ".BUF0"; | ||
| auto buf1Name = primaryName + ".BUF1"; | ||
|
|
||
| _target = backend->getRegisterAccessor<UserType>(buf0Name, numSamples, 0, accessFlags); |
There was a problem hiding this comment.
That's not how a decorator works. I think you are not allowed to simply replace the target you are supposed to decorate. The backend might (and probably will for push type registers) hold a copy of the original target.
What you have to do: When detecting a double buffer register for which an accessor is requested, to look into the catalogue and generate the buf0Name. The you create the BUF0 accessor, and decorate it with the second buffer and the handshake.
Was it like this in the original decorator?
| template class ChimeraTK::NumericDoubleBufferAccessorDecorator<int8_t>; | ||
| template class ChimeraTK::NumericDoubleBufferAccessorDecorator<uint8_t>; | ||
| template class ChimeraTK::NumericDoubleBufferAccessorDecorator<int16_t>; | ||
| template class ChimeraTK::NumericDoubleBufferAccessorDecorator<uint16_t>; | ||
| template class ChimeraTK::NumericDoubleBufferAccessorDecorator<int32_t>; | ||
| template class ChimeraTK::NumericDoubleBufferAccessorDecorator<uint32_t>; | ||
| template class ChimeraTK::NumericDoubleBufferAccessorDecorator<uint64_t>; | ||
| template class ChimeraTK::NumericDoubleBufferAccessorDecorator<long>; | ||
| template class ChimeraTK::NumericDoubleBufferAccessorDecorator<float>; | ||
| template class ChimeraTK::NumericDoubleBufferAccessorDecorator<double>; | ||
| template class ChimeraTK::NumericDoubleBufferAccessorDecorator<ChimeraTK::Void>; | ||
| template class ChimeraTK::NumericDoubleBufferAccessorDecorator<std::string>; | ||
| template class ChimeraTK::NumericDoubleBufferAccessorDecorator<ChimeraTK::Boolean>; |
There was a problem hiding this comment.
Please use the macro to loop all data types.
| BOOST_TEST(reg.doubleBuffer->offset == 0x80200000); | ||
| BOOST_TEST(reg.doubleBuffer->inactiveBufferRegisterPath == "/DAQ.DOUBLE_BUF.INACTIVE_BUF_ID"); | ||
| BOOST_TEST(reg.doubleBuffer->enableRegisterPath == "/DAQ.DOUBLE_BUF.ENA"); | ||
|
|
There was a problem hiding this comment.
Also the the .BUF0 and .BUF1 entries
killenb
left a comment
killenb
left a comment
There was a problem hiding this comment.
We also need a test that the DAQ region and the MacroPulseNumber are working together in a transfer group.
| BOOST_TEST(reg.channels[0].signedFlag == false); | ||
| } | ||
| { | ||
| auto reg = regs.getBackendRegister("DAQ.MACRO_PULSE_NUMBER"); |
There was a problem hiding this comment.
Tests for double buffering are missing here.
killenb
left a comment
killenb
left a comment
There was a problem hiding this comment.
Ooops, it seems I forgot to send the review. I hope it's done(?)
Major changes