LIFsCode / ELOC-3.0

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

Detect and Record Commands #49

Closed EDsteve closed 9 months ago

EDsteve commented 10 months ago

@LIFsCode @OOHehir I am in a session with Jospeh our app developer and implementing the commands. Do we already have a solution to start detecting? It's not clear yet if we go with the wiki or another way. Would be great if we can find out soon :)

Cheeers ED

OOHehir commented 10 months ago

@EDsteve

There is a command that starts & stops AI detection implemented in the commit I pushed last night

setAIMode (toggles) setAIMode on setAIMode off

But that was just my take on it.

EDsteve commented 10 months ago

@OOHehir Okay thanks for that. You have alread posted that info here. Sorry for asking again :)

@LIFsCode @OOHehir Maybe this will be a good place to discuss how we deal with recording / detecting commands and responses. There is no common ground yet of how we deal with these. And we need that for the app.

Commands

I think we have discussed two options now:

1. Option - As implemented right now in the master branch setRecordingmode (toggles between on and off) or setRecordingmode#mode=Off setRecordingmode#mode=On

setAIMode (toggles between on and off) or setAIMode#mode=On setAIMode#mode=Off

2. Option - This has been discussed briefly here Besides the toggling on/off we need to cover more combinations. So this would be an example of it: setRecordingmode#mode=recordOff setRecordingmode#mode=recordOn setRecordingmode#mode=recordOnEvent setRecordingmode#mode=recordOn_DetectOn setRecordingmode#mode=recordOff_DetectOn

The toggling is handy and easy to use for testing. But while implementing these in the app. We figuered that it is more fail safe using the full commands.

Responses

These are the responses i get right now with the last commit (84e9cff7429ac961effb74d5ac46b6502d974c45):

setRecordingmode#mode=On {"ecode" : 0, "error" : "ESP_OK", "cmd" : "setRecordMode", "payload" : {"recordingState" : 2}} -> Why 2?

setRecordingmode#mode=Off {"ecode" : 0, "error" : "ESP_OK", "cmd" : "setRecordMode", "payload" : {"recordingState" : 0}}

setAIMode#mode=off {"ecode" : 0, "error" : "ESP_OK", "cmd" : "setAIMode", "payload" : {"ai_state" : 0}}

setAIMode#mode=on {"ecode" : 0, "error" : "ESP_OK", "cmd" : "setAIMode", "payload" : {"ai_state" : 1}}

Because we have more than just four states. It would better to use the second option. And then use these resposnes? "recordingState" : 0 -> recordOff "recordingState" : 1 -> recordOn "recordingState" : 2 -> recordOnEvent "recordingState" : 3 -> recordOn_DetectOn "recordingState" : 4 -> recordOff_DetectOn

Not sure what is better code-wise. Please feel free to improve, change or find a better solutions. Cheeers ED

OOHehir commented 10 months ago

@EDsteve The reason for this

{"recordingState" : 2}} -> Why 2?

Is that the recording state is either

  1. disabled
  2. Triggered when the AI threshold is reached for a single recording
  3. Continous in the preselected lengths.

@EDsteve I'll have some time to implement these changes but checking with @LIFsCode if there's any specific implementation required?

LIFsCode commented 10 months ago
  1. Option - As implemented right now in the master branch setRecordingmode (toggles between on and off) or setRecordingmode#mode=Off setRecordingmode#mode=On

setAIMode (toggles between on and off) or setAIMode#mode=On setAIMode#mode=Off

  1. Option - This has been discussed briefly https://github.com/LIFsCode/ELOC-3.0/issues/10#issuecomment-1784090471 Besides the toggling on/off we need to cover more combinations. So this would be an example of it: setRecordingmode#mode=recordOff setRecordingmode#mode=recordOn setRecordingmode#mode=recordOnEvent setRecordingmode#mode=recordOn_DetectOn setRecordingmode#mode=recordOff_DetectOn

@EDsteve @OOHehir I would prefer option 2 as at least the recordOnEvent option would require both setting to match (AI on & recording "onEvent") it does not make sense if the AI is off. So to avoid any inconsistant config options this can be avoided to have only a single mode setting

So I vote for this: 2. Option - This has been discussed briefly https://github.com/LIFsCode/ELOC-3.0/issues/10#issuecomment-1784090471 Besides the toggling on/off we need to cover more combinations. So this would be an example of it: setRecordingmode#mode=recordOff setRecordingmode#mode=recordOn setRecordingmode#mode=recordOnEvent setRecordingmode#mode=recordOn_DetectOn setRecordingmode#mode=recordOff_DetectOn

OOHehir commented 10 months ago

This was mentioned previously but I'll include it here so it doesn't get forgotten:

@LIFsCode From what I can see the wav writer operation is set at boot up, from Bluetooth & GPIO0. I understand the current code uses the FreeRTOS queue. To access the enum modes of the wav writer I had to include the header (in BluetoothServer.hpp) for wav writer & the (globally scoped) object (to enable toggling the mode). At that point it seemed more efficient to set the mode directly rather than the queue but not sure if you want to proceed in that direction? If you wish to proceed in a solely based queue manner I can modifythe code. It is similar for the AI setting (discussed below).

OOHehir commented 10 months ago

@EDsteve OK, I've implemented these commands in the code.

setRecordingmode#mode=recordOff
setRecordingmode#mode=recordOn
setRecordingmode#mode=recordOnEvent
setRecordingmode#mode=recordOn_DetectOn
setRecordingmode#mode=recordOff_DetectOn

Tested very briefly, looks ok so far..

OOHehir commented 10 months ago

@EDsteve Perhaps something to sort out for this is the required response from the ELOC to the app. At the moment it sends back the command received (e.g. recordOnEvent)

EDsteve commented 10 months ago

@OOHehir Thanks. The responses look good to me. Also Joseph can work with it.

During testing i realized that we don't have a command to stop detecting. So i would think we also need setRecordingmode#mode=recordOff_DetectOff ?

OOHehir commented 10 months ago

@OOHehir Thanks. The responses look good to me. Also Joseph can work with it.

During testing i realized that we don't have a command to stop detecting. So i would think we also need setRecordingmode#mode=recordOff_DetectOff ?

Added here

EDsteve commented 10 months ago

@OOHehir Only now i have tested all the commands and this one doesn't seem to work: setrecordmode#mode=recordOn_DetectOn

The log says: E (459715) BtCmds: Invalid mode recordOn_DetectOn

OOHehir commented 10 months ago

@EDsteve Apologies, I've added that in the latest commit.

To confirm, for the commands:

setRecordingmode#mode=recordOff
setRecordingmode#mode=recordOn

Should there be any change to the AI/ detection? I.e. should it be turned off?

EDsteve commented 10 months ago

@OOHehir It's not possible in the app to change the recording state while detection in running. So recordOff and recordOff_DetectOff kind of fulfill the same purpose. I would still keep both modes though.

EDsteve commented 9 months ago

@OOHehir Sorry to come back to this. Joseph was a bit confused when implementing the commands into the app and wasn't sure about recordon / recordoff. And i realized that you actually asked the same question before. And my answer made it even more confusing.

To clear things up. These new commands will hopefully be less confusing and should fulfill all scenarios we need:

Command Recording Mode After Command AI Detection Mode After Command
recordOn_DetectOFF ON Off
recordOn_DetectOn ON ON
recordOff_DetectOn OFF ON
recordOff_DetectOff OFF OFF
recordOnEvent ON (when event detected) ON

Does that makes sense or am i making it even more confusing now? haha

OOHehir commented 9 months ago

@EDsteve This should be resolved now. If necessary you can re-open this.