dbinfrago / libpax

Apache License 2.0
21 stars 13 forks source link

1.0.0 RC1: Feature add ons and bugfixes (see comments in PR for details) #3

Closed cyberman54 closed 3 years ago

cyberman54 commented 3 years ago

@FlorianLudwig Can you re-run the benchmark with this version? Expectation is, that lookup speed significantly increased by using bitmap type style for storage of identifiers.

FlorianLudwig commented 3 years ago

As you removed the option to for the hash table completely, I assume you don't see a use case to for situations with less than 8kb fixed RAM usage?

Does make the code simpler not keeping around two versions, so I like that.

Regarding the benchmarks: @lifezoned4 you wrote the benchmark and I remember there were a few quirks, better you make the needed changes and interpretation of the numbers :)

@cyberman54 my collaegue is on vacation till monday, so the benchmark will be done then.

cyberman54 commented 3 years ago

Wifi sniffing neads devices with a certain power. It won't make sense to focus on devices with little RAM. Thus, i think we don't need a RAM minimzing version.

cyberman54 commented 3 years ago

An interesting finding, after running libpax with this PR on a device for ~14 days in cumulative counter mode: During the first days the device collected ~40.000 identifiers pretty fast, in a surrounding with stable ~800-1000 pax movements per day (measured with earlier paxcounter versions). After reaching this level the arrival rate of new identifiers decreased. After the device climbed to 64.500, the rate dropped significantly, what means the device does rarely see any more new identifiers.

Since libpax counts universal (= "random") MACs only, i did not expect this behaviour. I expected random MACs to arrive with an equal probability distribution. But at least for a certain range or number of 2 byte octets in the MAC address space {xx:xx:xx:yy:00:00 - xx:xx:xx:yy:FF:FF} this seems to be inapplicable. For whatever reason.

PS: the experiment proved, that there is no memory leak with libpax. Free heep space of the counting device remained constant all the time. Thus, bitmapping works. External PSRAM is no longer needed for dense environments.

image

/cc @FlorianLudwig

FlorianLudwig commented 3 years ago

@cyberman54 this is a really great test, nice to see it running stable.

The bitmap uses 2 bytes of the mac, so there are 2^16 = 65,536 possibilities. With every mac counted, the probability for collision gets higher. Therefore the counting rate will decrease over time.

Small simulation:

Simulation (x = added mac addresses, y = counter):

image

Overall the described behavior is as expected.

To allow for even higher counts in cumulative mode, we would need to use 3 bytes again as the previous paxcounter iteration did. This would allow higher counts and less collisions. This comes at expense of RAM usage:

A bitmask would use 2,097kb instead of the current 8kb. (which could theoretically count up to 2^24 = 1,677,7216!)

The "hash map"-like structure would be:

For non cumulative mode I believe using 2 bytes of the mac address is enough. For cumulative the argument could be made it would be worth using more than 2 bytes. For me it would boil down to how much is this mode used and is it worth implementing a second code path for it.

cyberman54 commented 3 years ago

Thanks for this reply. Since we are counting universal MACs, and these are random and thus there are collisions, i currently have no idea of a use case where cumulative count makes sense.

cyberman54 commented 3 years ago

added feature "set RSSI threshold" for BLE and WIFI separately

cyberman54 commented 3 years ago

@spmrider @FlorianLudwig ping

cyberman54 commented 3 years ago

Changed API for libpax_counter_init: uint16_t pax_report_interval -> uint16_t pax_report_interval_sec Milliseconds don't make sense here and cause problems with longer send cycles, see https://github.com/cyberman54/ESP32-Paxcounter/issues/828

cyberman54 commented 3 years ago

Now that it's in seconds, it would be good to also change this line: https://github.com/cyberman54/libpax/blob/475271ed75c39b7377ec7c14a385d3308a1f0ff6/src/main.cpp#L23

so that the user doesn't have to wait 3 hours for an output ^^

Done.

cyberman54 commented 3 years ago

@FlorianLudwig merge?

cyberman54 commented 3 years ago

include : i can't test this with espidf native, with arduino-esp32 it does compile.

This file sits here: https://github.com/espressif/esp-idf/blob/master/components/bt/include/esp32/include/esp_bt.h

FlorianLudwig commented 3 years ago

@cyberman54 how are you executing the test?

with only arduiono env:

pio test -e arduino

The tests do compile but fail for me:

test/libpax_test_cases.cpp:200:test_mac_add_bytes   [PASSED]
test/libpax_test_cases.cpp:201:test_collision_add   [PASSED]
test/libpax_test_cases.cpp:202:test_counter_reset   [PASSED]
test/libpax_test_cases.cpp:83:test_config_store:FAIL: Expected 72 Was 64    [FAILED]
after start: Current free heap: 80844
libpax should be running
test/libpax_test_cases.cpp:117:test_integration:FAIL: Expected 6 Was 0  [FAILED]
-----------------------
5 Tests 2 Failures 0 Ignored

Do they pass for you?

cyberman54 commented 3 years ago

@FlorianLudwig i did not execute the test, only compiled the library. It compiles succesfully with env = arduino. Will test the test soon.

cyberman54 commented 3 years ago

@FlorianLudwig This: test/libpax_test_cases.cpp:83:test_config_store:FAIL: Expected 72 Was 64 [FAILED] should be fixed now: test\libpax_test_cases.cpp:203:test_config_store [PASSED] ... thanks to the test cases we have here :-)

test\libpax_test_cases.cpp:204:test_integration [PASSED]
-----------------------
5 Tests 0 Failures 0 Ignored
cyberman54 commented 3 years ago

@FlorianLudwig PS: Please also note the new HCI branch

cyberman54 commented 3 years ago

@FlorianLudwig HCI version now passed integration test, too.

test\libpax_test_cases.cpp:216:test_integration [PASSED]
-----------------------
5 Tests 0 Failures 0 Ignored

But benchmark test does not compile due to linker error:

Processing * in arduino environment
--------------------------------------------------------------------------------------------------------------------------------------------------------------------Building...
.pio\build\arduino\test\libpax_benchmark.cpp.o:(.data.methodes_to_test+0x1c): undefined reference to `libpax_counter_reset'
.pio\build\arduino\test\libpax_benchmark.cpp.o:(.data.methodes_to_test+0x34): undefined reference to `add_to_bucket'
.pio\build\arduino\test\libpax_benchmark.cpp.o:(.data.methodes_to_test+0x3c): undefined reference to `reset_bucket' 
collect2.exe: error: ld returned 1 exit status
*** [.pio\build\arduino\firmware.elf] Error 1
cyberman54 commented 3 years ago

@FlorianLudwig how is the test_benchmark.cpp to be used?

cyberman54 commented 3 years ago

@FlorianLudwig in hci branch i cleaned up the includes & sdkconfig, now both versions arduino and espidf compile without errors.

FlorianLudwig commented 3 years ago

@cyberman54 so this MR can be closed and the RC2 one superseeds this one?

The benchmarks I would suggest to just delete. They were used to test the different data structures for storing counted mac addresses. As they got reworked in this MR the benchmark would need to be rebuild but I guess we do not need to benchmark the new structure again

cyberman54 commented 3 years ago

MR1 brings a can of important changes. MR2 includes MR1 and switches to VHCI command control, but this could affect the sniffing results especially with Wifi.