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

Handling Results throughout the crate #143

Merged
merged 7 commits into from
Feb 29, 2024

Conversation

ryan-summers
Copy link
Member

@ryan-summers ryan-summers commented Feb 28, 2024

Fixes #113 by handling all the various Result<> types and propogating them upwards to poll(). For now, they are simply logged. In the future, it could be helpful to push these outwards and have poll() return the Result<> type, however I'm hesitant to introduce a breaking API change right now. I'll spawn an issue for this.

This also fixes #58 by propagating the errors described to the new log macros.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!
I guess this does not need an entry in the changelog if the changes are all internal.

@mvirkkunen
Copy link
Collaborator

Just out of interest, did you make sure that e.g. the state changes in ControlPipe still work correctly even though the functions may return early now? I seem to remember that there were already some problems in there because error handling was "too strict" and in a lot of cases it might have been better to just ignore some of them because implementations can be racey and/or broken.

@ryan-summers
Copy link
Member Author

Just out of interest, did you make sure that e.g. the state changes in ControlPipe still work correctly even though the functions may return early now? I seem to remember that there were already some problems in there because error handling was "too strict" and in a lot of cases it might have been better to just ignore some of them because implementations can be racey and/or broken.

It's hard to say for sure or not because most of the errors would originate in the actual PHY implementation. That being said, I've tested this on both Linux and Windows with an STM32F4 and an H7, and both enumerated properly with both Release and Debug builds.

I think most of the racey and/or broken communication issues were control-pipe related and got fixed up in #142

While there's a possibility that issues could be getting introduced, the new logging features should make them simpler to work through when they occur.

@ryan-summers
Copy link
Member Author

Going to merge this in favor of moving forwards, but will definitely keep an eye out for regressions, thanks for bringing this up @mvirkkunen.

@ryan-summers ryan-summers merged commit af98f93 into master Feb 29, 2024
3 checks passed
@ryan-summers ryan-summers deleted the feature/result-propagation branch February 29, 2024 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants