Open aacuevas opened 1 year ago
A version with an extra parameter being an array of addresses to be able to do non-contiguous batch access could also be interesting. It's more niche, as it would mostly be used by client software as a way to speed up initialization operations, though. Contiguous access has real hardware counterparts
I like this idea. For simplicities sake, I dont want to implement special cases unless there is no way to get the same performance without the addition. So, for continuous registers, this makes sense. For non-continuous, it seems like the benefit is more nebulous. We should think about the function name though. Rather than "batch" maybe "block", "region", or "range".
I have been thinking about this a it might require more consideration than I initially thought. After all, the API does not deal directly with device access, but wraps over the ONI registers dedicated to this. So a single device register access involves 5 ONI register access, ONI_CONFIG_DEV_IDX, ONI_CONFIG_REG_ADDR, ONI_CONFIG_REG_VALUE, ONI_CONFIG_RW, ONI_CONFIG_TRIG
While the library could be made to concatenate calls of these 5 registers, with maybe some benefit in some drivers, the result would not be the expected improvement. Ideally, ONI register access itself should add a "continuous" mode, in which the config registers are set only once before sending the values in a continuous manner, which would be then accesses through the API.
We need to discuss this further.
Given the fact that this is such a performance pain point, it might be worth really putting some thought into this as you say. I really want to keep the stream definitions (read, write, reg io, info) as simple as possible. However, the reg io stream has a ton of "extra space" to support different forms of register access. I agree we should think about this more.
OK, so we discussed this today and the conclusion was that this should probably be a major API revision that modifies the signatures of oni_read_reg
and write_write_reg
to the following:
int oni_write_reg(const oni_ctx ctx, oni_dev_idx_t dev_idx, oni_reg_addr_t addr, oni_reg_val_t* value, size_t num)
int oni_read_reg(const oni_ctx ctx, oni_dev_idx_t dev_idx, oni_reg_addr_t addr, oni_reg_val_t* value, size_t num)
which have the original definitions as a subset of their functionality where num=1
. This will be a breaking change but will have large performance benefits. Bindings can probably accept this change without exposing massive changes to user code.
As a note, there is a proposal for the oni spec on open-ephys/ONI#7 that would create an optional hardware capability for doing this batch accesses in hardware. It should be the responsibility of the onidriver to discern if said capability is implemented, so these functions could use it if so, or emulate it by software if not, keeping the actual software signature the same.
I propose the addition of two new functions to the API:
int oni_write_reg_batch(const oni_ctx ctx, oni_dev_idx_t dev_idx, oni_reg_addr_t startaddr, unsigned int num, oni_reg_val_t* valuearray)
andint oni_read_reg_batch(const oni_ctx ctx, oni_dev_idx_t dev_idx, oni_reg_addr_t startaddr, unsigned int num, oni_reg_val_t* valuearray)
.These would read/write
num
registers starting fromstartaddr
. They would not be just looped wrappers for the individual calls, but the onidrivers would need to implement them.There are two reasons for this: 1- it's not uncommon for some devices to have contiguous addressed spaces dedicated to a single function. Many devices even include the possibility of performing batch transfers over their communication interface. 2- More importantly, register access usually comes with a communication overhead per access. Depending on the onidriver, it might be possible to greatly speed up access while joining multiple requests. If the onidriver didn't have the capability for batch transports, it can always loop individual calls internally.