Skip to content

Conversation

@adrian-kong
Copy link
Contributor

Description

Construct sbps from json fields rather than payload

JIRA Reference

https://swift-nav.atlassian.net/browse/DEVINFRA-189

adrian-kong and others added 5 commits July 6, 2022 09:50
…romfields

# Conflicts:
#	c/include/libsbp/version.h
#	docs/sbp.pdf
#	haskell/sbp.cabal
#	javascript/sbp/RELEASE-VERSION
#	package-lock.json
#	package.json
#	python/sbp/RELEASE-VERSION
#	rust/sbp/Cargo.toml
#	rust/sbp2json/Cargo.toml
@adrian-kong adrian-kong marked this pull request as ready for review July 7, 2022 01:44
@adrian-kong adrian-kong requested review from a team, notoriaga and silverjam as code owners July 7, 2022 01:44
where
D: serde::Deserializer<'de>,
{
let value = serde_json::Value::deserialize(deserializer)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

just to note there is probably a better way to do this. We can't use the derive + an untagged enum because some messages are subsets of other messages which causes messages to be deserialized as the wrong type. It might work if we added #[serde(deny_unknown_fields)]. But untagged still seems like the wrong approach as it just tries each variant in order which is inefficient as we know the type ahead of time.

typically you would not want to impl serde::Deserialize by using a specific data format like serde_json. But we don't actually expose a serde feature just a json one so any time we have serde we'll also have serde_json. I think there should be way to implement this with just serde. It'll look something like a deserializer that just grabs the id, and then a big match that forwards to the correct message deserializer. But we could look into that later if we ever decide to expose a plain serde feature

@notoriaga notoriaga force-pushed the adrian/fromfields branch from 88a94a0 to f3e7b8b Compare July 7, 2022 17:44
@notoriaga notoriaga force-pushed the adrian/fromfields branch from f3e7b8b to f052f91 Compare July 7, 2022 17:46
Copy link
Contributor

@silverjam silverjam left a comment

Choose a reason for hiding this comment

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

I think we should do some basic round trip testing so that we can ensure that we're at least converting all the messages in the input, doing this locally I'm seeing a lot of messages being dropped:

❯ ntripping --url swiftnav.jason:xxx@na.skylark.swiftnav.com:2101/SSR | swift rtcm3tosbp | pv >sbp.bin

❯ cargo run --quiet --release --bin sbp2json -- <sbp.bin >sbp.json

❯ cargo run --quiet --release --bin json2sbp -- --from-fields <sbp.json >sbp2.bin

❯ ls -l sbp.bin sbp2.bin
.rw-r--r-- 57k ubuntu  7 Jul 17:40 sbp.bin
.rw-r--r-- 20k ubuntu  7 Jul 17:41 sbp2.bin

❯ cargo run --quiet --release --bin sbp2json -- <sbp2.bin >sbp2.json

❯ wc -l sbp.json sbp2.json
   713 sbp.json
   133 sbp2.json
   846 total

@silverjam
Copy link
Contributor

silverjam commented Jul 8, 2022

Here's a copy of the input data I used: sbp.zip

@silverjam
Copy link
Contributor

Converting single messages doesn't seem to be able to round trip (the CRC changes), seems to be related to float formatting, but do we understand exactly why?

Input sbp.pretty.json:

{
  "crc": 8382,
  "length": 139,
  "msg_type": 138,
  "payload": "AQCgswYAqQgAAABAQDgAAAEAAACwMQAgJcIAYFVDAOATtgAwHzcAACS0AAA4Mz9rsTgDxC8+bJB/ihle6j8AAACIwKOIPwAA4EKqIbRAghZG/vvf/T/tJya2ldpAvi1DQwbHv+0/OsqeMQKd7z+kZHc0nGukPUDRqjkAAAStAAAAAKCzBgCpCEREAA==",
  "preamble": 85,
  "sender": 61568,
  "common": {
    "sid": {
      "sat": 1,
      "code": 0
    },
    "toe": {
      "tow": 439200,
      "wn": 2217
    },
    "ura": 2,
    "fit_interval": 14400,
    "valid": 1,
    "health_bits": 0
  },
  "tgd": 5.122274e-09,
  "c_rs": -41.28125,
  "c_rc": 213.375,
  "c_uc": -2.2035092e-06,
  "c_us": 9.488314e-06,
  "c_ic": -1.527369e-07,
  "c_is": 4.284084e-08,
  "dn": 3.6980111798662528e-09,
  "m0": 0.823986788277137,
  "ecc": 0.012031082296743989,
  "sqrta": 5153.665082931519,
  "omega0": 1.8671836788720673,
  "omegadot": -7.848184051224747e-09,
  "w": 0.9296603319472517,
  "inc": 0.9879160851617776,
  "inc_dot": 9.286101089098182e-12,
  "af0": 0.0003258083,
  "af1": -7.503331e-12,
  "af2": 0,
  "toc": {
    "tow": 439200,
    "wn": 2217
  },
  "iode": 68,
  "iodc": 68
}

Conversion:

❯ cargo run --quiet --release --bin json2sbp -- --from-fields <sbp.pretty.json | cargo run --quiet --release --bin sbp2json | jq

Output

{
  "crc": 659,
  "length": 139,
  "msg_type": 138,
  "payload": "AQCgswYAqQgAAABAQDgAAAEAAACwMQAgJcIAYFVDAOATtgAwHzcAACS0AAA4Mz5rsTgDxC8+bJB/ihle6j////+HwKOIPwAA4EKqIbRAghZG/vvf/T/tJya2ldpAvixDQwbHv+0/OsqeMQKd7z+kZHc0nGukPUDRqjkAAAStAAAAAKCzBgCpCEREAA==",
  "preamble": 85,
  "sender": 61568,
  "common": {
    "sid": {
      "sat": 1,
      "code": 0
    },
    "toe": {
      "tow": 439200,
      "wn": 2217
    },
    "ura": 2,
    "fit_interval": 14400,
    "valid": 1,
    "health_bits": 0
  },
  "tgd": 5.122274e-09,
  "c_rs": -41.28125,
  "c_rc": 213.375,
  "c_uc": -2.2035092e-06,
  "c_us": 9.488314e-06,
  "c_ic": -1.527369e-07,
  "c_is": 4.284084e-08,
  "dn": 3.6980111798662524e-09,
  "m0": 0.823986788277137,
  "ecc": 0.012031082296743987,
  "sqrta": 5153.665082931519,
  "omega0": 1.8671836788720673,
  "omegadot": -7.848184051224747e-09,
  "w": 0.9296603319472516,
  "inc": 0.9879160851617776,
  "inc_dot": 9.286101089098182e-12,
  "af0": 0.0003258083,
  "af1": -7.503331e-12,
  "af2": 0,
  "toc": {
    "tow": 439200,
    "wn": 2217
  },
  "iode": 68,
  "iodc": 68
}

Everything is almost the same, except for the ecc field.

@adrian-kong
Copy link
Contributor Author

  • fixed most of the messages, and deserialization issues but there is an interesting case where fields are all the same but CRC and payloads turn out different after running from-fields

@notoriaga
Copy link
Contributor

So I seem to be able to get it to round trip properly - starting with a single json message (which I generated from a file with one sbp message and ran cargo run --bin sbp2json -- <file.sbp)

{
  "crc": 33330,
  "length": 92,
  "msg_type": 139,
  "payload": "FANGIgQAMggAAIBAYAkAAAEAAAAAAECe1TkAAKAxAAAAuA7SQcEAAEBAz29jwQAA4GdQM3ZBAAAASw9hp0AAAADGWkGRwAAAAJDAUmbAAAB6NQAAAIAAAPq1CmQ=",
  "preamble": 85,
  "sender": 22963,
  "common": {
    "sid": {
      "sat": 20,
      "code": 3
    },
    "toe": {
      "tow": 270918,
      "wn": 2098
    },
    "ura": 4,
    "fit_interval": 2400,
    "valid": 1,
    "health_bits": 0
  },
  "gamma": 0,
  "tau": 0.0004074443,
  "d_tau": 4.656613e-09,
  "pos": [
    -2335773.4375,
    -10190458.0078125,
    23278854.4921875
  ],
  "vel": [
    2992.52986907959,
    -1104.3386459350586,
    -178.58600616455078
  ],
  "acc": [
    9.313226e-07,
    -0,
    -1.8626451e-06
  ],
  "fcn": 10,
  "iod": 100
}
cargo run --bin json2sbp -- --from-fields <test_data/test.json | cargo run --bin sbp2json | jq
{
  "crc": 33330,
  "length": 92,
  "msg_type": 139,
  "payload": "FANGIgQAMggAAIBAYAkAAAEAAAAAAECe1TkAAKAxAAAAuA7SQcEAAEBAz29jwQAA4GdQM3ZBAAAASw9hp0AAAADGWkGRwAAAAJDAUmbAAAB6NQAAAIAAAPq1CmQ=",
  "preamble": 85,
  "sender": 22963,
  "common": {
    "sid": {
      "sat": 20,
      "code": 3
    },
    "toe": {
      "tow": 270918,
      "wn": 2098
    },
    "ura": 4,
    "fit_interval": 2400,
    "valid": 1,
    "health_bits": 0
  },
  "gamma": 0,
  "tau": 0.0004074443,
  "d_tau": 4.656613e-09,
  "pos": [
    -2335773.4375,
    -10190458.0078125,
    23278854.4921875
  ],
  "vel": [
    2992.52986907959,
    -1104.3386459350586,
    -178.58600616455078
  ],
  "acc": [
    9.313226e-07,
    -0,
    -1.8626451e-06
  ],
  "fcn": 10,
  "iod": 100
}

@silverjam Where did your json blob originate from? I'm thinking this is from the --float-compat flag. the roundtrip tests use that flag which uses a different float formatter than the default serde one (https://docs.rs/ryu/latest/ryu/)

@silverjam
Copy link
Contributor

@silverjam Where did your json blob originate from? I'm thinking this is from the --float-compat flag. the roundtrip tests use that flag which uses a different float formatter than the default serde one (https://docs.rs/ryu/latest/ryu/)

The JSON blob was generated using the code in this repo, so it definitely wasn't round tripping before (with a single message). The message I'd had issues with earlier is working now though:

❯ jq . sbp.json                                                                                                          (base)
{
  "crc": 8382,
  "length": 139,
  "msg_type": 138,
  "payload": "AQCgswYAqQgAAABAQDgAAAEAAACwMQAgJcIAYFVDAOATtgAwHzcAACS0AAA4Mz9rsTgDxC8+bJB/ihle6j8AAACIwKOIPwAA4EKqIbRAghZG/vvf/T/tJya2ldpAvi1DQwbHv+0/OsqeMQKd7z+kZHc0nGukPUDRqjkAAAStAAAAAKCzBgCpCEREAA==",
  "preamble": 85,
  "sender": 61568,
  "common": {
    "sid": {
      "sat": 1,
      "code": 0
    },
    "toe": {
      "tow": 439200,
      "wn": 2217
    },
    "ura": 2,
    "fit_interval": 14400,
    "valid": 1,
    "health_bits": 0
  },
  "tgd": 5.122274e-09,
  "c_rs": -41.28125,
  "c_rc": 213.375,
  "c_uc": -2.2035092e-06,
  "c_us": 9.488314e-06,
  "c_ic": -1.527369e-07,
  "c_is": 4.284084e-08,
  "dn": 3.6980111798662528e-09,
  "m0": 0.823986788277137,
  "ecc": 0.012031082296743989,
  "sqrta": 5153.665082931519,
  "omega0": 1.8671836788720673,
  "omegadot": -7.848184051224747e-09,
  "w": 0.9296603319472517,
  "inc": 0.9879160851617776,
  "inc_dot": 9.286101089098182e-12,
  "af0": 0.0003258083,
  "af1": -7.503331e-12,
  "af2": 0,
  "toc": {
    "tow": 439200,
    "wn": 2217
  },
  "iode": 68,
  "iodc": 68
}

❯ cargo run --quiet --release --bin json2sbp -- <sbp.json >sbp.bin

❯ cargo run --quiet --release --bin json2sbp -- --from-fields -- <sbp.json >sbp2.bin

❯ sha256sum sbp.bin sbp2.bin
9500d4d184422939ab6b39884665f48b31c63dd2024fc2b325249053a14fa28e  sbp.bin
9500d4d184422939ab6b39884665f48b31c63dd2024fc2b325249053a14fa28e  sbp2.bin

❯ cargo run --quiet --release --bin sbp2json <sbp.bin | jq .crc
8382

❯ cargo run --quiet --release --bin sbp2json <sbp2.bin | jq .crc
8382

@silverjam
Copy link
Contributor

silverjam commented Jul 8, 2022

So, with these changes things seem to be a lot closer:

❯ ntripping --url swiftnav.jason:xxx@na.skylark.swiftnav.com:2101/SSR | swift rtcm3tosbp | pv >ssr.sbp                                                                                       

❯ ls -l ssr.sbp
.rw-r--r-- 32k ubuntu  8 Jul 13:13 ssr.sbp

❯ cargo run --quiet --release --bin sbp2json <ssr.sbp >sbp.json

❯ cargo run --quiet --release --bin json2sbp <sbp.json >sbp.bin

❯ cargo run --quiet --release --bin json2sbp -- --from-fields <sbp.json >sbp2.bin

❯ sha256sum sbp.bin sbp2.bin
a3e5ae780d81ae066ce3327fc8459e84d0b450fe87d10aa68d35de75318d58fa  sbp.bin
a3e5ae780d81ae066ce3327fc8459e84d0b450fe87d10aa68d35de75318d58fa  sbp2.bin

❯ ls -l *.bin
.rw-r--r--  32k ubuntu  8 Jul 13:14 sbp.bin
.rw-r--r--  32k ubuntu  8 Jul 13:14 sbp2.bin

@notoriaga
Copy link
Contributor

I renamed roundtrip.json to roundtrip_float_compat.json and regenerated roundtrip.json without using the float compat flag. Using that data the roundtrip test is passing

@silverjam
Copy link
Contributor

@notoriaga I'm working on updating the cargo.toml's -- I can update the benchmark thresholds too, these are probably slowdowns from changing the float parsing

Copy link
Contributor

@silverjam silverjam left a comment

Choose a reason for hiding this comment

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

:shipit:

@notoriaga
Copy link
Contributor

Pushed one more change to remove some panics around deserializing into fixed length strings

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.

4 participants