Deprecate bootloader handshake + send SBP protocol version #154
Conversation
… in bootloader handshake and heartbeat.
|
Test PASSed. |
|
lgtm |
spec/yaml/swiftnav/sbp/system.yaml
Outdated
There was a problem hiding this comment.
I believe that the intention for the version stuff would go into the reserved field of the heartbeat message?
There was a problem hiding this comment.
@mookerji are we using a version string? If we we're packing the version elements (major, minor, sub-minor) into the reserved field, that might be possible - but if we're using an opaque string, that doesn't seem like it will work.
/cc @henryhallam
There was a problem hiding this comment.
We've currently got a bit of a mix of encoding vs raw strings for versioning info throughout Piksi. It would probably be good to standardize this. Off the top of my head:
- STM FW Version
- String
- NAP FW Version
- String
- Note: Older versions were a 4 byte identifier
- Bootloader Version
- String
- Note: Older versions were a 1 byte identifier
- HW Version
- Currently a 4 byte identifier. Should we change this to a string?
- SBP Protocol Version
- This PR would introduce it as a string
There was a problem hiding this comment.
@mfine: Not a version string. In this case, this would put the SBP protocol minor version into the reserved field, like 42.
There was a problem hiding this comment.
The NAP FW string has already proved to be troublesome to use as a string programmatically - the first 100 lines of main.c in piksi_firmware are a NAP version comparison function (https://github.com/swift-nav/piksi_firmware/blob/master/src/main.c#L57).
I'd vote for making the SBP protocol version a tuple of u8s or something else thats easy to compare programmatically and is more efficient to send in frequent message such as the MSG_HEARTBEAT.
Standardization would be good eventually but I don't think we should get too deep into that right now? I'd say we should make the best choice for the SBP version and use that going forward and we can improve the other versioning schemes later.
For the FW versions I think strings make more sense anyway as they are primarily just for displaying to the user.
There was a problem hiding this comment.
So we'd have 24 bits in the reserved field of the heartbeat message? And use a similar fields scheme for the new bootloader handshake? Makes sense to be more formal and rigid with the SBP protocol version. Any objections?
|
👍 for standardizing. |
|
👍 for standardizing |
|
Test PASSed. |
|
Do we need 24 bits for the SBP protocol version? Seems like a lot. Would be nice if it was byte aligned as well for the heartbeat. What about 16 bits from [8:24)? |
|
@cbeighley k, had it in my head we wanted a 3 tuple - but yeah, just needs a |
|
Test PASSed. |
|
Test PASSed. |
|
bump on any reviews here. |
There was a problem hiding this comment.
Do you think deprecated messages should be public? I can see arguments both ways...
There was a problem hiding this comment.
Yeah, maybe they should inherit their previous state? Like if they were public, they stay public? And vice versa.
|
Test PASSed. |
|
Test PASSed. |
|
Looking at this just now. |
|
Looks good. Merge at will. |
|
Test PASSed. |
Deprecate bootloader handshake + send SBP protocol version
…RCE_DIR (#154) (#1330) Automated PR by Jenkins. If CI has passed successfully, merge away! **cmake** 38cf865b -> 12b7f037 - 12b7f037 : Fix TEST_SRCS property - use CMAKE_SOURCE_DIR (swift-nav/cmake#154) This pull request was created by https://jenkins.ci.swift-nav.com/job/CI%20Infra/job/submodule-update/14233/.
5 participants
Setup a new bootloader message from the device, containing both the bootloader version and the SBP protocol version as strings. Also add SBP protocol version as a string to the heartbeat.
There's a lot more changes that need to happen on top of this - collecting feedback before embarking on those changes.
/cc @fnoble @cbeighley @mookerji