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

Panic at 'attempt to subtract with overflow'; due to cpr_nl returning invalid values #3

Closed
wiseman opened this issue Jan 22, 2021 · 1 comment

Comments

@wiseman
Copy link
Contributor

wiseman commented Jan 22, 2021

Page 47 of http://www.anteni.net/adsb/Doc/1090-WP-14-09R1.pdf says

For latitudes at or near the N or S pole, where the above formula would either be undefined or yield NL(lat) = 0, the value returned by the NL( ) function shall be 1.

but cpr_nl as currently defined can return 0.

    #[test]
    fn cpr_nl1() {
        assert_eq!(cpr_nl(89.9), 1);
        assert_eq!(cpr_nl(-89.9), 1);
    }
running 1 test
thread 'cpr::tests::cpr_nl1' panicked at 'assertion failed: `(left == right)`
  left: `0`,
 right: `1`', src/cpr.rs:105:9

This can lead to a panic when decoding positions:

    #[test]
    fn cpr_calculate_position2() {
        let even = CPRFrame {
            position: Position {
                latitude: 108011.0,
                longitude: 110088.0,
            },
            parity: Parity::Even,
        };
        let odd = CPRFrame {
            position: Position {
                latitude: 75050.0,
                longitude: 36777.0,
            },
            parity: Parity::Odd,
        };
        let position = get_position((&even, &odd)).unwrap();
        assert_approx_eq!(position.latitude, 88.91747426178496);
        assert_approx_eq!(position.longitude, 101.01104736328125);
    }
running 1 test
thread 'cpr::tests::cpr_calculate_position2' panicked at 'attempt to subtract with overflow', src/cpr.rs:88:23
stack backtrace:
   0: rust_begin_unwind
             at /rustc/d98d2f57d9b98325ff075c343d2c7695b66dfa7d/library/std/src/panicking.rs:493:5
   1: core::panicking::panic_fmt
             at /rustc/d98d2f57d9b98325ff075c343d2c7695b66dfa7d/library/core/src/panicking.rs:92:14
   2: core::panicking::panic
             at /rustc/d98d2f57d9b98325ff075c343d2c7695b66dfa7d/library/core/src/panicking.rs:50:5
   3: adsb::cpr::get_lat_lon
             at ./src/cpr.rs:88:23
   4: adsb::cpr::get_position
             at ./src/cpr.rs:74:22
   5: adsb::cpr::tests::cpr_calculate_position2
             at ./src/cpr.rs:126:24

I found this by running the decoder over about 14 billion pings I've recorded from the Los Angeles area. In that many pings you get some weird data, and this one cropped up about 4 million pings into the test, where I accidentally used even and odd frames that were broadcast about 10 hours and many miles apart (and may be corrupted/weird anyway).

wiseman added a commit to wiseman/adsb that referenced this issue Jan 22, 2021
cpr_nl was returning 0 in some cases, which could lead to panics. This
change switches cpr_nl from a trigonometry-based implementation
to (essentially) a table lookup.

I added a couple unit tests, and a criterion benchmark for cpr_nl to
validate the table-based speedup.

On my machine this version is, in the worst case situation, 40%
faster. In the best case it's 89% faster.

```
cpr_nl_high_lat         time:   [10.764 ns 10.795 ns 10.830 ns]
                        change: [-40.716% -39.922% -39.255%] (p = 0.00 < 0.05)
                        Performance has improved.

cpr_nl_low_lat          time:   [2.2473 ns 2.2617 ns 2.2767 ns]
                        change: [-89.576% -89.479% -89.386%] (p = 0.00 < 0.05)
                        Performance has improved.
```

Fixes issue asmarques#3.
wiseman added a commit to wiseman/adsb that referenced this issue Jan 22, 2021
cpr_nl was returning 0 in some cases, which could lead to panics. This
change switches cpr_nl from a trigonometry-based implementation
to (essentially) a table lookup.

I added a couple unit tests, and a criterion benchmark for cpr_nl to
validate the table-based speedup.

On my machine this version is, in the worst case situation, 40%
faster. In the best case it's 89% faster.

```
cpr_nl_high_lat         time:   [10.764 ns 10.795 ns 10.830 ns]
                        change: [-40.716% -39.922% -39.255%] (p = 0.00 < 0.05)
                        Performance has improved.

cpr_nl_low_lat          time:   [2.2473 ns 2.2617 ns 2.2767 ns]
                        change: [-89.576% -89.479% -89.386%] (p = 0.00 < 0.05)
                        Performance has improved.
```

Fixes issue asmarques#3.
@asmarques
Copy link
Owner

Fixed by #4.

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

No branches or pull requests

2 participants