- Notifications
You must be signed in to change notification settings - Fork81
Initial modernization (WIP)#20
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…18 and latest dependencies. Errors now conforming to std::error::Error.
Cargo.toml Outdated
| itertools = "0.10" | ||
| libc = "0.2" | ||
| nix = "0.23" | ||
| clap = "2.33" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd consider moving thebin/can.rs toexamples/can.rs or creating a workspace with two separate crates to avoid the dependencies onanyhow andclap for projects that use the socketcan crate only for the library part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, you're probably right. Normally I don't worry about it, but for a small, embedded crate like this the overhead is significant. I prefer to keep examples short and easy to understand, and keep what I consider "useful utilities" separate. A workspace seems a bit too complicated. Maybe just hide the utility(s) behind a feature flag?
Also I was debating the name. "can" seens a bit generic, though I want to keep it short and simple. "rcan"? "canrs"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I went with a feature flag.
lachlansneff commentedFeb 28, 2022
Hi! Super excited to see this get worked on! |
tobz left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A few small comments since it seems like code reorganization is somewhat in scope given the changes so far.
src/lib.rs Outdated
| impl error::Error for CanSocketOpenError { | ||
| fn description(&self) -> &str { | ||
| match *self { | ||
| CanSocketOpenError::LookupError(_) => "can device not found", | ||
| CanSocketOpenError::IOError(ref e) => e.description(), | ||
| } | ||
| } | ||
| fn cause(&self) -> Option<&error::Error> { | ||
| match *self { | ||
| CanSocketOpenError::LookupError(ref e) => Some(e), | ||
| CanSocketOpenError::IOError(ref e) => Some(e), | ||
| } | ||
| } | ||
| } | ||
| impl error::Error for CanSocketOpenError {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Since I can't comment on the enum itself since it's not in the diff... could it make more sense to move this error to thesrc/err.rs file since that seems to be the intended spot for most, if not all, error declarations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A few small comments since it seems like code reorganization is somewhat in scope given the changes so far.
Yes, I'm assuming that this will be a major update with lots of refactoring, and that breaking changes are OK as long as there's an overall consensus that we're moving in the right direction.
could it make more sense to move this error to the src/err.rs
Yes, I was thinking the same thing when I reviewed this work recently (the branch is from last year). I may have been thinking to just do a little at a time? But since this PR includes error refactoring as a goal, this is probably a good place to make the change.
Also... I typically prefer to export a single error type from a library, because it tends to make things simpler when you layer libraries on top of each other. And these days I just do it withthiserror. But I wasn't sure how people would feel about either of those things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, this is just pedantic/gut reaction nits, but it seems likely the error design might continue to change as you/whoever keeps working on refactoring/modernization.
End of the day, I guess this is more stream of consciousness than actual suggestion. :P
src/nl.rs Outdated
| @@ -148,50 +45,84 @@ impl CanInterface { | |||
| /// Open CAN interface by name | |||
| /// | |||
| /// Similar to `open_if`, but looks up the device by name instead | |||
| pub fn open(ifname: &str) -> Result<CanInterface, nix::Error> { | |||
| pub fn open(ifname: &str) ->result::Result<Self, nix::Error> { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why theresult::Result? It's not competing with any other type being brought into scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Did I do this? 😕 If so, it was likely because I was preparing for a crate-specificError and associated Result, and didn't get to it. (yet?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's what I get for commenting on a WIP PR. :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No,@tobz, you're right. I was going down that path, but received some feedback saying to keep it the way it is for now, and didn't clean up properly. I'm trying to find the balance of what people want to do in the next release or two.
src/lib.rs Outdated
| } | ||
| } | ||
| } | ||
| impl error::Error for ConstructionError {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same comment as above about this error cohabiting with the others insrc/err.rs.
abalmos commentedMar 1, 2022
@fpagliughi Just an FYI -- I am working on a J1939 SocketCan library and in the process preparing PRs to both It is a night/weekend project, so it will take a little more time to properly prepare. I will try to remember to comment back here when the PRs are up. |
XVilka commentedJul 22, 2022
Looking forward to getting this merged. If something should be done to make it possible - I can help. |
tcbennun commentedNov 13, 2022
Would be great to see this work continue. I have a fairly pressing need for this, so I might end up forking in the meantime. Other things that would be great to add would be 1) CAN-FD support and 2) async support (simple tokio wrapper would do, |
FYI regarding the async support there istokio-socketcan . Since this relies on the socketcan crate but progress is stuck I guess a fork is a good idea for CAN-FD. I think@nnarain also worked on a fork, so might make sense to look at that. |
Apologies for the slow start on this as I've had write access for a few months now. It got queued up in line behind a few other open-source projects. But I promise I'll get started within the next few weeks when I get back to a whole bunch of CAN work. |
This is an initial update to start work on this package again; to update the dependencies and start moving toward a more recent version of Rust.
neliv0.6nixdependency to latest v0.23