Skip to content
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

Suggestion: Batch register access #8

Open
aacuevas opened this issue Dec 22, 2022 · 6 comments
Open

Suggestion: Batch register access #8

aacuevas opened this issue Dec 22, 2022 · 6 comments
Labels
enhancement New feature or request

Comments

@aacuevas
Copy link
Collaborator

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) and int 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 from startaddr. 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.

@aacuevas aacuevas added the enhancement New feature or request label Dec 22, 2022
@aacuevas
Copy link
Collaborator Author

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

@jonnew
Copy link
Collaborator

jonnew commented Dec 22, 2022

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".

@aacuevas
Copy link
Collaborator Author

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.

@jonnew
Copy link
Collaborator

jonnew commented Dec 30, 2022

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.

@jonnew
Copy link
Collaborator

jonnew commented Dec 5, 2023

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.

@aacuevas
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants