-
Notifications
You must be signed in to change notification settings - Fork 4
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
API Version 2.0 #32
base: master
Are you sure you want to change the base?
API Version 2.0 #32
Conversation
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.
Thanks for the PR! I think using dlopen
makes sense given the reasoning provided.
ni-fpga-sys/src/lib.rs
Outdated
|
||
use core::ffi::c_char; | ||
use std::ffi::CString; | ||
impl PartialEq for DlOpenError { |
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.
It looks like this could be derived instead? Is there a reason it isn't?
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.
The enum in the dlopen package isn't PartialEq. Which is why I have to use a newtype at all.
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.
Hmm. I'm curious why you need to compare a dlopen::Error
in the first place now.
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 its a possible result in the high level Error enum, that requires it to be PartialEq
ni-fpga-sys/src/lib.rs
Outdated
} | ||
|
||
impl StatusHelper for Status { | ||
fn to_result(self) -> Result<(), Status> { |
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.
By convention, *-sys
crates only handle FFI, with any higher-level abstractions, like turning a status code into a Result
, being left to a companion crate. This allows users to use a different abstraction whilst still being able to link to the same system library.
Given we're changing this to dynamically loading the library however, this concern doesn't really exist any more. As such it might make more sense to move all this back into the ni-fpga
crate. @connorworley thoughts?
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 can move all this code up, and just leave the FFI at the low level.
Only higher level API's for specific registers need to be mut. The low level ones should just always work
FYI I'm not very involved in FRC lately, so please feel free to take the library in whatever direction you'd like & don't wait on me to review. |
This will allow passing the types around
This is going to be a long term PR, but I do want it open. |
This is going to be a 2.0 API.
Its a MUCH better API, does a much better job at expressing rust's memory safety, and has a lot more supported features.