search

LIFsCode / ELOC-3.0

Firmware for ELOC 3.0 Hardware
MIT License
3 stars 3 forks source link

Merging heapDebugging into master - please review 'sdkconfig.defaults' & 'nvs.csv' #76

Closed OOHehir closed 7 months ago

OOHehir commented 7 months ago

@LIFsCode I believe I've merged most of the required from heapDebugging into master. When you get a chance could you please review the files below to see what you'd like to retain?

  1. sdkconfig.defaults
  2. nvs.csv
LIFsCode commented 7 months ago

@OOHehir Thanks for the merge.

I'm not sure how familiar you are with the git cherry-pick feature. I guess this would be the more simple and clearer option for picking the important bugfix changes to master branch, but that's just for the future.

I added a fix for starting the AI detection that's all. The sdkconfig.defaults looks good. In the end we can discuss on which core we want to have the BT running. Since this had no effect on the crashs we could keep it on core 0 which is under less load. But that's a story after the Release of the batch of ELOCs on thursday.

I also did a few tests today (also with extended heap poisoning) and didn't run into any crashs. @EDsteve Could you run your tests with 6d0dd72 This is most likely the version to be released on the ELOCs.

I still have a few questions/remarks for this merge. But these are not for now, first let @EDsteve deploy the first batch of ELOCs and then we can start cleaning things up a bit:

  1. You still delete the EI Task which may be potentially harmfull if it is triggered via TaskNotify. Is there any reason for this? Because I guess since the system must support running recording & detection in parallel. We could also just leave the tasks created throughout the whole runtime. If the task is blocked via a xTaskNotifyWaitor xQueueReceive with portMAX_DELAY timeout the task will seldomly run and thus won't affect power consumption. Having the tasks created throughout the whole runtime also has a positive effect on heap fragmentation. Or what do you think about runtime static threads?
  2. I must admit I don't get this commit 941e785. As I see it this commit has 2 parts:
    • Changes in I2SMEMSSampler.cpp --> not notiying the AI task when it's not running --> solves problem with a deleted task (see my point 1 here) That's the actual bugfix
    • Changes in main.cpp: This does not make sense in my opinion. Using the intermediate variable before assigning the global should not affect execution behavior. Or if it does it is only in a random timing related behavior and thus not really stable. So in my opinion these changes could be reversed.
OOHehir commented 7 months ago

@LIFsCode To discuss the EI task I'll open a discussion rather than continuing it here.

With regards to the local intermediate variable you're correct. I suspected the bug was a behavior where:

  1. 'ai_run_enable' changed to in ElocCommands.cpp (line 353)
  2. I2SMEMSampler.cpp (line 296) checks ai_run_enable & based on value sends/ doesn't send a notification via ei_TaskHandler
  3. (some point later..) at line 961 in main.cpp the notification from ElocCommands.cpp is received & acted upon.

I believed the bug was a mismatch between 2 & 3 above & could be solved by coordinating the change ai_run_enable with the change in edgeImpulse.set_status(EdgeImpulse::Status::xxxxx. ) using the intermediate variable.

However a better solution was introducing the bool inference->status_running & appears to have solved this. I'll remove this local variable after first deployment.