colonelpanichacks / m5paper-Wardriver

Basic M5paper wardriver.
3 stars 2 forks source link

limited to tracking 150 devices then restarts #6

Open ZeroChaos- opened 1 month ago

ZeroChaos- commented 1 month ago

deviceList is created as a 150 element array https://github.com/colonelpanichacks/m5paper-Wardriver/blob/main/m5paperwardriver/m5paperwardriver.ino#L45

deviceIndex is ++ for every device, with no size checking https://github.com/colonelpanichacks/m5paper-Wardriver/blob/main/m5paperwardriver/m5paperwardriver.ino#L144

when deviceIndex hits 151 (which would be device number 152 due to starting at 0) it will try to index into a 150 item array at position 151 and crash https://github.com/colonelpanichacks/m5paper-Wardriver/blob/main/m5paperwardriver/m5paperwardriver.ino#L139

There are a few possible improvements and solutions here.

Improvement 1 : I'm testing one improvement right now, and that's to increase the size of the deviceList array. There isn't enough memory to go above about 1000, so I set it to 750 and added a minimum free memory line to the status bar. I'm testing now to make sure it doesn't have a problem when it reaches that many devices. I suspect it will be fine since the memory allocation is performed when the array is made not per item added.

That said, this solution doesn't actually 'fix' anything, it just takes longer to hit the max and crash.

Improvement 2: An additional improvement could be made to remove old devices from the deviceList, this could both speed up the display as it would only need to show the recently seen devices instead of "all time", and it would help keep us under the limit. Again, this doesn't "solve" the issue, but it would drastically reduce it.

Actual solution: To solve the problem we can easily add a check to line 144 and detect when we are trying to raise the index out of the array. I'm happy to write and PR that, but I don't know what you would like to do in that case. So, what would you like to do? :-) I'm happy to implement it if I have the skill. My thoughts are "wipe the device list and start over" or "restart the system" (without crashing but the effect would be the same)

ZeroChaos- commented 1 month ago

I added the simplest possible solution to this in https://github.com/colonelpanichacks/m5paper-Wardriver/pull/4 by simply checking if the deviceIndex is higher than the array size and setting deviceIndex to 0. This expires data like a FIFO but makes the total counts unreliable (I set those to zero with the index). There is probably a better solution but this prevents the crash and that's a win.

ZeroChaos- commented 1 month ago

I was thinking the best way to solve this is to add a last seen timestamp to the Device struct. Using that we could do two things which would help a lot.

  1. We could not show data older than some timeout, call it 60 or 120 seconds. This would cut down on the number of devices we are showing, and make the display more useful.
  2. We could use the timestamp to expire the data which hasn't been seen for the longest amount of time first. I'd probably clean it out in batches of 50 or something for performance reasons, but this would allow us to expire actually old data instead of just first seen data.

Based on some memory debugging it looks like we actually have plenty of memory to do things like this, would you be okay if I increased the size of the Device struct and added a timestamp?