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

Use own proc macro to generate code to deal with packets #7

Merged
merged 22 commits into from
Mar 24, 2020

Conversation

Dushistov
Copy link
Contributor

Features:

@lkolbly

Problems I found during conversation to usage of proc macro that I would like to disscuss:

  1. I replaced the end of NavPosVelTime
    before:
    pos_dop: u16,
    reserved2: u16,
    pub reserved3: u32,

after:

    pos_dop: u16,
    reserved1: [u8; 6],
    heading_of_vehicle: i32,
    magnetic_declination: i16,
    magnetic_declination_accuracy: u16,

the last version is match documentation, while previous one not,
not sure how this works before

  1. NavPosVelTime uses mm/s as speed units while NavVelNed uses cm/s
    according to documentation, but previous code for some reason treats NavVelNed
    as mm instead of cm, is it problem with documentation or with code?
    And side note NavStatus have similar fields with NavPosVelTime , but only one bit representation match according to documentation, all other fields has different format, though the same name and the same purpose.

  2. Why negative nanoseconds values converted to 0, may be negative also valid value, or why they use signed integer instead of unsigned one?

  3. May be use chrono function to validate date instead of convert it without check?

What missed:

And comment from previous review:

The create_packet signature looks good to me (would it make sense to call it serialize

This is against idea that Builder is just the way to create packet, so you can not serialize builder, there is no sense in this operation, but you can create packet from builder. That's why I don't call it serialization.

@lkolbly
Copy link
Collaborator

lkolbly commented Mar 11, 2020

full conversation of code with features=serial

Don't spend too much effort on this part - I want to totally rewrite how the serial library parts work, so if anything gets sticky with that, feel free to just comment it out entirely.

I'll look more at this later when I get a sec, but in general where the documentation and the code disagree I trust the documentation more.

@Dushistov
Copy link
Contributor Author

Fixed issue with UbxPacketRequest that I found during
testing with real device.

so if anything gets sticky with that, feel free to just comment it out entirely

I did almost that.

@Dushistov Dushistov force-pushed the codegen-real-deal branch 2 times, most recently from 13e4138 to ed85046 Compare March 13, 2020 15:43
Features:
- removes unnecessary dependicies
- more type safe code
- faster parser: 3x
fixes ublox-rs#1
Usefull for SPI, where we need provide read buffer simultaneously
with write buffer
@lkolbly lkolbly self-assigned this Mar 14, 2020
Copy link
Collaborator

@lkolbly lkolbly left a comment

Choose a reason for hiding this comment

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

I haven't gotten to the derive crate yet, but looking good so far.

ublox/src/ubx_packets/packets.rs Outdated Show resolved Hide resolved
ublox/src/ubx_packets/packets.rs Outdated Show resolved Hide resolved
ublox_derive/src/types.rs Outdated Show resolved Hide resolved
ublox/src/ubx_packets/packets.rs Outdated Show resolved Hide resolved
ublox/tests/parser_tests.rs Outdated Show resolved Hide resolved
ublox/src/ubx_packets/packets.rs Show resolved Hide resolved
ublox/src/ubx_packets/packets.rs Show resolved Hide resolved
ublox/src/ubx_packets.rs Outdated Show resolved Hide resolved
ublox/src/ubx_packets.rs Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
ublox_derive/src/input.rs Outdated Show resolved Hide resolved
ublox_derive/src/input.rs Outdated Show resolved Hide resolved
ublox_derive/src/input.rs Show resolved Hide resolved
ublox_derive/src/input.rs Show resolved Hide resolved
ublox_derive/src/output.rs Outdated Show resolved Hide resolved
ublox_derive/src/output.rs Show resolved Hide resolved
ublox_derive/src/output.rs Show resolved Hide resolved
ublox_derive/src/output.rs Show resolved Hide resolved
ublox_derive/src/output.rs Outdated Show resolved Hide resolved
ublox_derive/src/output.rs Outdated Show resolved Hide resolved
@Dushistov Dushistov requested a review from lkolbly March 24, 2020 12:10
Copy link
Collaborator

@lkolbly lkolbly left a comment

Choose a reason for hiding this comment

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

Just that one comment about collecting over results, and then this looks good to me. Thanks for all your hard work on this!

@lkolbly lkolbly merged commit 5d013ef into ublox-rs:master Mar 24, 2020
@Dushistov Dushistov deleted the codegen-real-deal branch March 24, 2020 16:14
lkolbly pushed a commit that referenced this pull request Oct 15, 2022
- Add size_fn to specify sizing of dynamic message when serializing packets.

- Add serialization and code gen for iterators and structs

Co-authored-by: Steven Meyer <smeyer@fastmail.com>
Co-authored-by: Jason Mobarak <jason@swift-nav.com>
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.

separate protocol and implementation?
2 participants