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

Remove generic arguments for read and write buffer. #21

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

de-vri-es
Copy link
Member

@de-vri-es de-vri-es commented Dec 30, 2024

Main goal is to reduce the mental overhead of using the Client and Device types. Having three generic types can be quite cumbersome to handle.

Downside: this solution doesn't allow using owned fixed-size arrays as buffer anymore. The only way to use a simple fixed size array is by borrowing it or by boxing it (requires feature = "alloc").

@omelia-iliffe: Can you see what you think about this solution too?

/edit: Hmm, maybe the buffers can be totally removed from the struct... The only reason they need to be owned/borrowed by the bus is because one serial_port.read() could read multiple responses at once. But we usually discard all data before sending a new command.

Only in the case of single command multiple response is this a real problem. But maybe the new iterator-like design for multi-response-instructions can keep the buffer borrowed until the last response is read...

@omelia-iliffe
Copy link
Contributor

omelia-iliffe commented Dec 31, 2024

I agree it is getting busy but losing the flexibility of a simple generic is a shame. It could be reduced down to one as I don't see a scenario where you would want the read and write buffer to be different types. Could just be Buffer?

Removing them would be even better, although I can't quite see a way for it to work without having a buffer somewhere.

The message return size could be a const generic but that might further complicate things. And actually you can't calculate the exact StatusPacket size before receiving the header due to byte-stuffing.

@de-vri-es
Copy link
Member Author

de-vri-es commented Dec 31, 2024

I'm thinking that it might be possible to use a buffer on the stack whenever we actually perform a read or write.

The stack buffer can be owned by the function that does the read/write. But we will not be able to return Response<&'a [u8]> anymore, since the buffer will be destroyed once the function returns...

The other alternative is that you have to pass in a buffer as argument to all functions that need it (which is basically every function). That makes the API closer to raw socket calls, which is good for performance and flexibility, but bad for usability.

One Buffer argument does help already... With the design of this PR we have a lifetime generic instead of two buffer types. Maybe one buffer type instead of a lifetime is also fine. Although lifetimes can often be left out in function signatures, generic type arguments can not.

@omelia-iliffe
Copy link
Contributor

Is it possible to have different default generics for different cfgs? That have allocc could default to Vec and no_std to [u8]. Then advanced users who need more control could still override them.

@de-vri-es
Copy link
Member Author

Hmm, yeah, that should be possible. I like that, users wont normally be confronted with generics they don't care about, but if you do care you can still specify them. I'll rework this PR later!

@omelia-iliffe
Copy link
Contributor

Could also make the SerialPort generic default on std?
When having multiple default generics the order is important.
If we have Client<Buffer = Vec<u8>, SerialPort = serial2::SerialPort> users can't specify the second generic (SerialPort) without also specifying the first (Buffer).

@de-vri-es
Copy link
Member Author

de-vri-es commented Jan 1, 2025

Could also make the SerialPort generic default on std?

This one is a bit more difficult, because that means we need to have no default without the serial2 feature. Changing the default can be done rather easily with a trick like this:

#[cfg(feature = "alloc")]
type DefaultBuffer = Vec<u8>;

#[cfg(not(feature = "alloc"))]
type DefaultBuffer = &'static mut [u8];

struct Client<SerialPort, Buffer = DefaultBuffer> {
   ...
}

But removing the default based on lack of a feature means duplicating the struct definition. Still possible though, but slightly more risky.

When having multiple default generics the order is important.
If we have Client<Buffer = Vec<u8>, SerialPort = serial2::SerialPort> users can't specify the second generic (SerialPort) without also specifying the first (Buffer).

Yeah, we have to consider what value most users will want to change. On std + serial2 I think most users wont want to change anything. But if they change anything it will probably be the buffer. On embedded, users have to specify the serial port because there is no default implementation.

I'm leaning towards putting the SerialPort generic first, and the Buffer second.

@de-vri-es de-vri-es force-pushed the remove-buffer-generics branch 2 times, most recently from a3a08bb to 79ab7da Compare January 1, 2025 10:16
@de-vri-es
Copy link
Member Author

Updated the PR. Now it:

  • Merges the ReadBuffer and WriteBuffer into a single Buffer argument.
  • Specifies a default for the Buffer argument: Vec<u8> is "alloc" is enabled, &'static mut [u8] otherwise.
  • Specifies a default for the SerialPort argument if the "serial2" feature is enabled (using a bit of macro hackery to avoid the risk of having two struct definitions).
  • Moves the SerialPort generic argument to the front to allow embedded users to specify the SerialPort while still using the default buffer type.

@de-vri-es de-vri-es force-pushed the remove-buffer-generics branch from 79ab7da to 33792e3 Compare January 1, 2025 10:22
@omelia-iliffe
Copy link
Contributor

Yes I think this is a great solution and I love the way you have implemented it.

Unfortunately, we will also have to do this for the SyncRead struct and any others we add. But I think its worth it.

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

Successfully merging this pull request may close these issues.

2 participants