iotile / coretools

Core python tools for building and using IOTile based devices
GNU General Public License v3.0
14 stars 7 forks source link

Feat add bled broadcast mode #978

Closed timothyfroehlich closed 4 years ago

timothyfroehlich commented 4 years ago

Overview & Major Changes

This PR changes the behavior of the Bled112's Reader Thread to add an optional Broadcast V2 Deduplicator.

Additional Notes

My tests show an extremely small amount of dropped packets, even with 75 Pods, close to the number of dropped packets that we expect due to the Bled122 switching between the channels it scans. In a five minute test, it missed no updates from a Pod that updates every second, and only one update from a Pod that updates every half second (which means that it missed three to four packets in a row from the Pod, but this only occurred once). These are massive improvements over the previous code, which missed 40 updates from the 1Hz Pod and 355 from the 0.5Hz Pod.

My tests run in real time and give me good summaries, but don't put out any easy to read graphs yet. I may enhance them to output graphs next week.

timothyfroehlich commented 4 years ago

@timburke Could I get a review?

timburke commented 4 years ago

@timothyfroehlich One more thing to check is that this still works when active-scan is set to True. In that case, I believe, even though the v2 advertisements don't have scan response information, the bled112 sends a packet with all 0s in it as a scan response since the host computer can't know whether to expect a scan response or not.

There are two potential issues:

  1. Those scan responses get through and cut into the performance improvement because there are still a lot of them coming through
  2. The scan-response parsing logic in the BLED112 adapter code expects that a scan-response will match a previously seen advertisement. I'm not sure if it behaves properly if one slips through whereas the advertisement was filtered out.
timothyfroehlich commented 4 years ago

@timothyfroehlich One more thing to check is that this still works when active-scan is set to True. In that case, I believe, even though the v2 advertisements don't have scan response information, the bled112 sends a packet with all 0s in it as a scan response since the host computer can't know whether to expect a scan response or not.

There are two potential issues:

  1. Those scan responses get through and cut into the performance improvement because there are still a lot of them coming through
  1. The scan-response parsing logic in the BLED112 adapter code expects that a scan-response will match a previously seen advertisement. I'm not sure if it behaves properly if one slips through whereas the advertisement was filtered out.

This does all point to potential performance increases by disabling active scan, but I believe that was tried before. Perhaps not with my current tester.

timothyfroehlich commented 4 years ago

I'm seeing some not-good behavior related to the changes I've made to the serial read code. When the device is overwhelmed with incoming data, and the deduplication of broadcasts is disabled, the device no longer processes any packets, they all end up in buffers that increase until the program crashes. Ending the simulation allows the device to begin processing those buffered packets and forwarding them. I'm going to work on my simulator and test harness so I can more easily run tests, and make sure that these read loop changes are worth it.

timothyfroehlich commented 4 years ago

I've reverted the read loop changes. In my performance tests I did see some slight performance gains from it but overall I judged it not good enough to make the change. I'm looking to merge this in now.