Serde for Iterators, Added new messages #40
Conversation
Refactor DwrdIter, R4 doMes, RxmRawx bitflags
- 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>
.github/workflows/rust.yml
Outdated
| @@ -21,7 +21,7 @@ jobs: | |||
| - name: Install MSRV | |||
| uses: actions-rs/toolchain@v1 | |||
| with: | |||
| toolchain: 1.57.0 | |||
| toolchain: 1.63.0 | |||
There was a problem hiding this comment.
Hey! Sorry for the delayed review.
Do we need 1.63 for this change, or can we get by with 1.57? I'm trying to keep the MSRV around a year old or so.
There was a problem hiding this comment.
I don't think so, can try reverting this. Besides that, is everything all good?
There was a problem hiding this comment.
I just commented about the ublox_cli changes, but then yeah everything looks good. (it looks like there might be some merge conflicts, I haven't gotten a chance to look at them yet)
| @@ -9,7 +9,7 @@ repository = "https://github.com/lkolbly/ublox" | |||
| readme = "../README.md" | |||
|
|
|||
| [features] | |||
| default = ["std"] | |||
| default = ["std", "serde"] | |||
There was a problem hiding this comment.
Should we disable serde by default?
There was a problem hiding this comment.
I think serde by default should be fine.
.github/workflows/rust.yml
Outdated
| @@ -17,7 +17,7 @@ jobs: | |||
| steps: | |||
| - uses: actions/checkout@v2 | |||
| - name: Install libudev | |||
| run: sudo apt-get install -y libudev-dev | |||
| run: sudo apt-get update && sudo apt-get install -y libudev-dev | |||
There was a problem hiding this comment.
We need a build variant that ensures that the library can build with serde enabled, correct?
There was a problem hiding this comment.
We also need a matrix that tests we can build no_std and no alloc, for now I think we can survive with just testing the default case and we'll add that later.
|
@silverjam I think CI is currently enabled for this PR? (also, I should be able to merge this this weekend, hopefully) |
|
There are some merge conflicts though, maybe that's blocking CI. |
|
@silverjam Sorry for the delay, yeah apparently it wasn't enabled, not sure what was up with that. Anyway I enabled CI and it looks like we just need to |
|
@lkolbly Ok, builds and tests are passing locally for me -- looks like you'll need to approve the CI one more time |
|
Thanks for getting this in @lkolbly! |
This PR adds some of missing messages (before recent merges) and code-gen for serde serialize implementations, helpful for serializing iterators.
List of new messages:
Serialization outputs includes the class id and message id*