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

support msp version 2 #3

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

support msp version 2 #3

wants to merge 26 commits into from

Conversation

amfern
Copy link

@amfern amfern commented Mar 27, 2020

extended the library to support msp v2 messages

src/packet.rs Outdated Show resolved Hide resolved
src/packet.rs Outdated Show resolved Hide resolved
src/packet.rs Outdated Show resolved Hide resolved
@wucke13
Copy link

wucke13 commented Apr 5, 2020

@amfern Are you interested in co-maintaining a Rust MSP library?

@amfern
Copy link
Author

amfern commented Apr 6, 2020

@wucke13 Appreciate your review, and yes i am interested in co-maintaining, you can fork this project and i will submit this PR to you. as you wish.

@amfern amfern force-pushed the master branch 2 times, most recently from 7de0fa7 to 3c04e7a Compare April 6, 2020 13:47
@amfern
Copy link
Author

amfern commented Apr 6, 2020

@wucke13 i am still working on updating this PR, so don't bother looking re-reviewing it yet

@amfern
Copy link
Author

amfern commented Apr 6, 2020

@wucke13 finished updating the PR, see if there anything i missed, thanks

Cargo.toml Outdated Show resolved Hide resolved
@@ -12,6 +12,8 @@ extern crate alloc;

extern crate packed_struct;

extern crate crc_any;
Copy link

Choose a reason for hiding this comment

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

I prefer the use style imports, anyhow this surely better fits the rest of the code the way it is.

Copy link
Author

Choose a reason for hiding this comment

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

i just tried to make it work, not so proficient with rust to have opinion yet

src/packet.rs Outdated Show resolved Hide resolved
pub fn packet_size_bytes_v2(&self) -> usize {
9 + self.data.len()
}

Copy link

Choose a reason for hiding this comment

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

This is rather unfulfilling. I would prefer to have the MspVersion enum in use to determine the actual size of package. However, the MSP version is currently not actually a part of a package...

Copy link
Author

Choose a reason for hiding this comment

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

I agree, maybe create something like this https://github.com/magiclen/crc-any/blob/b3ab9f45eee99d47534707703e442246206ef105/src/lib.rs#L181 for MspPacket, but i rather keep this change out of this PR and create different one in the future

@amfern amfern force-pushed the master branch 4 times, most recently from b6b357e to 5a62e9d Compare April 11, 2020 15:06
MspParserState::FlagV2 => {
// uint8, flag, usage to be defined (set to zero)
self.state = MspParserState::CommandV2;
self.packet_data = Vec::with_capacity(2);
Copy link

Choose a reason for hiding this comment

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

This breaks the code for no_std. Consider static arrays internally and passing slices (&[u8]) for input. Vec requires a heap, which is not given in no_std without additional crates.

Copy link
Author

Choose a reason for hiding this comment

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

packet_data is already Vec<>, so that means no_std is already broken.
also how do i go about validating no_std is working, i tried creating dummy project with #![no_std], similar to https://github.com/hashmismatch/packed_struct.rs/tree/master/packed_struct_nostd_tests. but it just won't compile.

Copy link
Author

Choose a reason for hiding this comment

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

i think we should fork this repo, and address no_std in separate PR

@amfern amfern force-pushed the master branch 2 times, most recently from af5e3b6 to 2e680a9 Compare June 18, 2020 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants