Conversation
|
LedController-testLedSequence-share confirmed working on |
…uence test. updated led factory test prompts
|
|
produceconsumerobot
left a comment
There was a problem hiding this comment.
Generally just style comments.
Biggest question is around LedStatus multiple source of truth with the NCP driver.
| struct LedStatus | ||
| { | ||
| bool state = false; | ||
| bool isChanged = false; |
There was a problem hiding this comment.
isChanged is slick, though I wonder how much value keeping track of isChanged practically has. It saves a little bit of CPU, but I think we have to tune our ISR cycles based on worst-case, which would be changing all 3 LEDs.
There was a problem hiding this comment.
Does this lead to a multiple source of truth with each the NCP driver and the LED controller separately keeping track of state?
Would it have been better to keep LedStatus only in the NCP driver?
There was a problem hiding this comment.
Here was my thought process:
- Since we now have the LED controller, it becomes the user facing element and all
seek/updatecalls will only go to the controller, so I thought it was necessary for the controller to hold the state of LEDs. - Yes, there can be a mismatch in the LED states as stored in the controller and the driver, but, the state update is only unidirectional. The EmotiBit class updates the LedController which in turn updates the driver. Since EmotiBit only has access to the LedController, the multiple source of truth although present is not realized.
- Furthermore, I think we we want to remove the MSOT, we should drop the storage element in the driver. And use it purely for communicating with the IC. I think, in the future, that is the way to proceed. Hence, I am going to leave it as is now because removing it from the LedController may go against the final change we want to make.
Let me if there is something I missed in the reasoning.
There was a problem hiding this comment.
@nitin710 why not push the SSOT down to the driver?
Is there anything uniquely EmotiBit about LED states?
There was a problem hiding this comment.
After thinking about it, I think I agree it should be pushed down to the driver 😅. Below is the reason.
EDIT: consider a emotibit variant where the LEDs are directly controlled by GPIOs (for example), it makes more sense the the LedController holds the LED states. Need to dig deeper. But maybe, in that case, there is a separate driver that controls LEDS using GPIOs? (notes for future self. Don't hate me.)
TLDR;
If we keep the state storage in the driver, the driver can be used asynchronously, but if the storage lives in a wrapper class above the driver, then the driver can only be used synchronously without additionally creating a wrapper class.
| EmotiBit stock FW only allows I2C communication during ISR. | ||
| * @return true is successful, else false | ||
| */ | ||
| bool update(); |
There was a problem hiding this comment.
Is there a reason you use update() here but utilize the pass-through send() fn in the NCP driver?
Generally consistency can be nice to keep things intuitive.
There was a problem hiding this comment.
send is now hidden from the EmotiBit class and I was trying to play with the set and update terminology. Update made more sense looking from EmotiBit class's perspective.
That's the only reason, but if you think this may lead to confusion with the usage, the change would look something like
EmotiBitLedController::update()->EmotiBitLedController::send()EmotiBitLedController::_updateNcp()->EmotiBitLedController::_sendNcp()
There was a problem hiding this comment.
My recollection is that we specifically chose send when we adapted the driver. I think perhaps it was to distinguish sending internal register to the LEDs (unidirectional) vs updating internal registers to match LEDs (bidirectional), but I can't totally remember. I like the idea of creating consistency across the EmotiBit ecosystem. Is there anywhere else update() is used other than emotibit.h/cpp though?
There was a problem hiding this comment.
Found the following places we use update
- updating button press
- updating data (reading form the sensors)
- NVM controller uses it with both read/write.
I think we have run out of time to make the change and test it again. I will add this to an issue for my future self.
|
Created issue #317 for unresolved comments. Merging PR. |
TestingBootup V6 M0Results
Serial LogI2C data pin: 11 EmotiBit HW version: V06a Sensor setup: Set Samples averaged: Initializing EDA... Loading EDA calibration... Initializing SD card...0,card initialized. Loading configuration file: config.txt Setting up WiFi Feather M0 detected. Switch to EmotiBit Oscilloscope to stream Data Bootup V6 ESP32Results
Serial LogI2C data pin: 27 EmotiBit HW version: V06a Sensor setup: Set Samples averaged: Loading EDA calibration... Initializing SD card...0,card initialized. Loading configuration file: /config.txt Setting up WiFi The data acquisition is executing on core: 1 HUZZAH32 Feather detected. Switch to EmotiBit Oscilloscope to stream Data Starting control connection to server: 192.168.0.101 : 3133 ... connected |
Description
Requirements
Issues Referenced
Notes for Reviewer
Testing
6into Serial input when prompted (setNo Line Endingoption). (This way you can start the example on a V5 and force the version to be V6 for testing the change of address for the IC)1.11.1.feat-LedController.0.Results
Feature Tests
Shared files
Checklist to allow merge
masterDIGITAL_WRITE_DEBUG= false (if set true while testing)Screenshots: