neXenio / BLE-Indoor-Positioning

Multilateration using bluetooth beacons
Apache License 2.0
430 stars 129 forks source link

Reverse beacon sorting #145

Closed jobhh closed 5 years ago

jobhh commented 5 years ago

The beacons were sorted from weakest RSSI to strongest. In IndoorPositioning.updateLocation() this resulted in the strongest beacons being discarded if there existed more than 3 beacons that are below the minimumRssiThreshold.

In commit debc2b0f59caf671bae254245c70ffe250466249 the call to Collections.reverse was, I assume accidentally, removed. Introducing this behaviour. Since beacon sorting isn't used elsewhere I think it's safe to reverse the sorting algorithm, but reintroducing Collections.reverse would also work.

codecov-io commented 5 years ago

Codecov Report

Merging #145 into dev will increase coverage by 0.03%. The diff coverage is 40%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #145      +/-   ##
============================================
+ Coverage     39.42%   39.45%   +0.03%     
- Complexity      235      236       +1     
============================================
  Files            40       40              
  Lines          1357     1361       +4     
  Branches        132      132              
============================================
+ Hits            535      537       +2     
- Misses          771      773       +2     
  Partials         51       51
Impacted Files Coverage Δ Complexity Δ
...exenio/bleindoorpositioning/ble/beacon/Beacon.java 51.4% <ø> (ø) 30 <0> (ø) :arrow_down:
...exenio/bleindoorpositioning/IndoorPositioning.java 0% <0%> (ø) 0 <0> (ø) :arrow_down:
...io/bleindoorpositioning/ble/beacon/BeaconUtil.java 39.13% <50%> (+1.03%) 10 <1> (+1) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2146283...17deea5. Read the comment docs.

Steppschuh commented 5 years ago

Hey, thanks for the heads up and the proposed solution.

I would prefer the fix to not be a breaking change (I know projects that are using the current Beacon.RssiComparator). We could create two new comparators (with more meaningful naming) and mark the current one a deprecated, or we could re-add the reversing.

jobhh commented 5 years ago

Are RssiHighToLowComparator and RssiLowToHighComparator acceptable?

Steppschuh commented 5 years ago

That looks better. I would suggest Ascending and Descending instead, though.

It would probably make sense to move these two new comparators to new classes instead of them being inner classes of Beacon. What do you think, @Uwinator ?

Also, when deprecating stuff, always add a reference to the class that should be used instead to the JavaDoc. For instance: @deprecated use {@link AscendingRssiComparator} instead

jobhh commented 5 years ago

Like so? Adding them to BeaconUtil seemed like an valid option to me.