-
Notifications
You must be signed in to change notification settings - Fork 97
Rust: callback based message handler #1070
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
Conversation
4e67b94 to
1a694f1
Compare
rust/sbp/src/link.rs
Outdated
| const MESSAGE_TYPES: &'static [u16] = &[T::MESSAGE_TYPE]; | ||
|
|
||
| fn from_sbp(msg: SBP) -> Self { | ||
| msg.try_into().unwrap() |
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 this be fallible? At least should use .expect unless we never expect .unwrap to fail?
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 shouldn't ever fail, because from_sbp is only called after checking that the message was in MESSAGE_TYPES. So if you create a custom event and set the wrong messages types you could hit this error. So an expect is probably a good idea to make that more clear
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.
Agree, expect could point out that it should only happen if there's a programming 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.
Well so actually this shouldn't ever error, this blanket impl makes all ConcreteMessages usable as events triggered by that messages type. So baring a logic error in the Link (i.e. sending an event to the wrong handler or something) this should never happen. Replaced the unwrap with a panic that will include the offending message just in case
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.
👍
rust/sbp/src/link.rs
Outdated
| self.stateless_link.clone() | ||
| } | ||
|
|
||
| pub fn send_state<M>(&self, state: &S, msg: M) -> bool |
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.
| pub fn send_state<M>(&self, state: &S, msg: M) -> bool | |
| pub fn send_with_state<M>(&self, state: &S, msg: M) -> bool |
Maybe?
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.
👍
rust/sbp/src/link.rs
Outdated
| let link = source.link(); | ||
| let count = RefCell::new(0); | ||
|
|
||
| link.register(|count: &RefCell<usize>, _: SBP| { |
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 this test receiving an ObsMsg struct?
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.
Yup good catch
No description provided.