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

Add Read overloads for passing in buffer #102

Merged
merged 1 commit into from
Aug 12, 2024
Merged

Add Read overloads for passing in buffer #102

merged 1 commit into from
Aug 12, 2024

Conversation

TimonLukas
Copy link
Contributor

@TimonLukas TimonLukas commented Aug 11, 2024

Implements #101

Warning: I did tested this only briefly (with a short bit of temporary code in HidApi.Tester). Can you recommend a good way to test it beyond that?

The method signatures had to slightly change compared to what we last discussed in the issue:

public interface IDevice {
    int Read(Span<byte> buffer);
    int ReadTimeout(Span<byte> buffer, int milliseconds);

    // removed based on discussion
    // ReadOnlySpan<byte> Read(int maxLength);
    // ReadOnlySpan<byte> ReadTimeout(int maxLength, int milliseconds);
}

As the Read might not fill the whole buffer, the new overloads have to return the read length. I believe this is another good reason to leave the previous signatures intact, as they prevent this mistake.

Copy link
Owner

@badcel badcel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks already pretty good I added some remarks for small improvements.

Unfortunately I don't have devices to test this too. A detailed look must be sufficient.

If you have done the changes please squash all your changes into one commit to avoid having several commits for such a minimal change.

src/HidApi.Net/Device.cs Outdated Show resolved Hide resolved
src/HidApi.Net/Device.cs Outdated Show resolved Hide resolved
src/HidApi.Net/Device.cs Outdated Show resolved Hide resolved
src/HidApi.Net/Device.cs Outdated Show resolved Hide resolved
@TimonLukas
Copy link
Contributor Author

TimonLukas commented Aug 11, 2024

There, now it's clean. Sorry for the oversights!

@badcel
Copy link
Owner

badcel commented Aug 12, 2024

The code is not free of obsolete whitespaces, this is why the build fails. Please run dotnet format in the src folder to fix the issue automatically.

Please amend your changes to the last commit to avoid an additional commit.

After this the PR is good to go 🚀.

Co-authored-by: Marcel Tiede <[email protected]>

Remove previous methods (as discussed) & fix doc comments

Add Read overloads for passing in buffer
@TimonLukas
Copy link
Contributor Author

There we go. Unfortunately dotnet format leads to an unhandled exception in the project, seems my current installation is somehow broken - but I believe I fixed the previous issue.

@badcel badcel merged commit d9b3c34 into badcel:main Aug 12, 2024
3 checks passed
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