-
Notifications
You must be signed in to change notification settings - Fork 29
Add register and channel R/W functions, fix ARM i8 cast bug, and extend stream result reporting #44
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
base: master
Are you sure you want to change the base?
Conversation
…s time, errors, and flags
kevinmehall
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.
Thanks! This covers 3 out of the 4 open issues!
I think it'd be best to split the StreamResult changes into a standalone PR because that might require more discussion. It's also a breaking API change, which is fine, we can release a v0.5, but there may be other breaking changes that we want to include with that. The example utilities in bin/ will have to be updated for that API.
src/device.rs
Outdated
| ))?; | ||
|
|
||
| Ok(len as usize) | ||
| // Is it better to return this StreamResult or just make the values public off of rx_stream |
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.
See #6 for prior discussion on this if you haven't already seen that thread. I'll have to revisit that before deciding if this is what we want.
| // TODO: sensors | ||
|
|
||
| // TODO: registers | ||
| // Write a register |
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.
For rustdoc:
| // Write a register | |
| /// Write a register |
It would be better to add more details like on what this might be used for and what the arguments mean.
src/device.rs
Outdated
| pub fn write_register<S: Into<Vec<u8>>>(&self, key: S, addr: u32, value: u32) -> Result<(), Error> { | ||
| let key = CString::new(key).expect("key must not contain null byte"); |
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.
Device.h uses name instead of key; should probably be consistent with that, since users are probably going to have to go looking at the driver code to figure out what to pass here.
| SoapySDRDevice_setMasterClockRate(self.inner.ptr, clock_rate); | ||
| check_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.
In Device.h, this returns an error code or 0 for success, so I think this should be
| SoapySDRDevice_setMasterClockRate(self.inner.ptr, clock_rate); | |
| check_error(()) | |
| check_ret_error(SoapySDRDevice_setMasterClockRate(self.inner.ptr, clock_rate)) |
| } | ||
| } | ||
|
|
||
| // Master clock rate |
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.
Should be a doc comment
A few bigger updates here but much of the code change is from running cargo format, actual changes include: