Feat WiFi creds over serial by nitin710 · Pull Request #283 · EmotiBit/EmotiBit_FeatherWing

nitin710 · 2023-09-14T14:47:19Z

Description

Adds provision to write WiFi credentials to config file over serial input
Fixes issue where any serial input key could activate Debug mode

Requirements

Introducing typetags to support WiFi creds over serial EmotiBit_XPlat_Utils#25

Issues Referenced

Provision to enter network creds to config file while EmotiBit is in operation #274

Documentation update

Documentation on adding WiFi credentials through serial EmotiBit_Docs#115

Notes for Reviewer

For details about handshaking, see SD Card Serial Add Wifi Credentials API Diagram drawing in drive.
sample output

Successfully entered config file edit mode.
@FV,1.9.0~
Options available in this mode:
1. Add a wifi credential. Usage: @WA,{"ssid":"SSSS","password" : "PPPP"}~
  - Replace SSSS with network name and PPPP with network password
2. List wifi credentials. Usage: @LS,~
3. Delete Wifi Credential. Usage: @WD,<credential number>~
  - use List option to get a wifi list. Then use the number in the list to delete a credential

>>>>>>>>>>>[serial input] @WA,{"ssid":"emoti-net","password" : "emotibit-rocks"}~
typetag: WA
payload: {"ssid":"emoti-net","password" : "emotibit-rocks"}
Parsed output: 
ssid: emoti-net
password: emotibit-rocks


config file exists. Appending to file.
@AK,WA~
Updated file contents:
{
  "WifiCredentials": [
    {
      "ssid": "emoti-net",
      "password": "emotibit-rocks"
    }
  ]
}


>>>>>>>>>>>[serial input] @LS,~
typetag: LS
payload: 
##################################
config file credetials:
0. emoti-net : emotibit-rocks
1. SSSS : PPPP
##################################
@AK,LS~


>>>>>>>>>>>[serial input] @WD,1~
typetag: WD
payload: 1
Deleting entry: 1. SSSS
@AK,WD~

Testing

Results

Synopsis of results

Test	Feather ESP32	Feather M0
Add credentials when config.txt does not exist	✔️	✔️
Add credentials when config.txt fails parsing	✔️
Add cred, config exists, can be parsed	✔️	✔️
delete valid option	✔️	✔️
delete invalid option	✔️	✔️
add enterprise cred	✔️	✔️

Feature Tests

[✔️ ] WiFi creds serial interface: Add cred, config not exist
[✔️ ] WiFi creds serial interface: Add cred, config exists, cannot be parsed
[✔️ ] WiFi creds serial interface: Add cred, config exists, can be parsed
[✔️ ] WiFi creds serial interface: Add cred, config exists, can be parsed, more than 12 creds
[✔️ ] WiFi creds serial interface: delete valid option, config exists, can be parsed
[✔️ ] WiFi creds serial interface: delete invalid option, config exists, can be parsed
[✔️ ] WiFi creds serial interface: delete option, config does not exists
[✔️ ] WiFi creds serial interface: Add enterprise cred, config exists, can be parsed

Steps to test

Use the serial monitor to edit credentials in the config file
Handy test inputs
- Add a credential: @WA,{"ssid":"ssid_name","password" : "network_password"}~
- Delete a credential: @WD,<number>~
- List creds: @LS,~

Shared files

Checklist to allow merge

All dependent repositories used were on branch master
Firmware
- Set testingMode to TestingMode::NONE
- Set const bool DIGITAL_WRITE_DEBUG = false (if set true while testing)
- Update version in EmotiBit.h
- Update library.properties to the correct version (should match EmotiBit.h)
doxygen style comments included for new code snippets
Required documentation updated

… (incomplete).

nitin710 · 2023-09-26T14:41:24Z

WIP

✔️ Needs a clearer Serial interface
Add
- ✔️ details on options available when the mode is entered
- [stretch goal] move Sd functions to a new file?
- ✔️ "pretty" the output when using list command
- ~~output the new config file contents when a delete is performed, (same as add)~~

nitin710 · 2023-09-26T21:19:15Z

Moving this provision to a different class.

Class name options
- EmotiBitSdController
  - failure cases
    - If storatge is not an Sd-Card
      - NAND flash or
      - EEPROM
      - FRAM
- EmotiBitStorageController // As per discussion, storage might be a superset of NVM. a better word needed

Current functions getting affected

{
  bool setupSdCard(int chipSelPin, bool loadConfig);
  bool loadConfigFile(String filename); // need to pass wifi object handle
  bool writeSdCardMessages(File &dataFile, String &stringToWrite);
  void printConfigInfo(File &file, String &stringToWrite)
  void processWiFiConfigInputs(String &configFileName);
}

New class

class UserAccessibleStorage
{
  bool setup(enum hwVersion, string sku); 
  bool loadConfigFile(String filename, EmotiBitWiFi &emotibitWifi); 
  bool writeToStorage(File &dataFile, String &stringToWrite);
  void writeConfigInfo(File &file, String &stringToWrite);
  void processWiFiConfigInput(String &configFileName);
  bool createDataFile(String filename);
}

nitin710 · 2023-09-27T17:33:12Z

Discussion points

storage is a superset of NVM
Removing the word "EmotiBit"
Removing the word "controller"

nitin710 · 2023-10-06T15:07:29Z

possible stored data types

exposed to user
- config file
- data csv file
- data info file
- setup log
- ?Derivative metric typetags (defined by users)
- Derivative metric pipelines
- sampling rates
- base filename modifier
- device name
- disabe/enable sensors sets
- disable/enable typetags
- crash event logging
- debug log
emotibit private
- hardware serial number/identification
- Sensor calib data
  - EDA
- hardware specific sensor-constellation profile
  - maybe this is hardcoded in the EEPROM not if/else in code?
- crash core dump

Organizing into categories

DataWriter (class)
- writeCsv
- write Info
Data Reader (class) // future implementation. Probably plays with implementing reading data files from SD-Card using WiFi
- readCsv
- readInfo
ConfigurationManager (decoupled from other classes like DataWriter)(class)
- config File (currently)
  - in the future, we may split this into
    - loadWifiCreds (exclusively used by EmotiBitWiFi)
    - processConfigWiFiInputs (called buy EmotiBit)
      - emotibit.configurationManager.processWiFiCredsInputs()
- Derivative metric typetags (defined by users)
- Derivative metric pipelines
- sampling rates
- base filename modifier
- device name
- disabe/enable sensors sets
- disable/enable typetags
- hardware serial number (emotibit private)
- sensor calib (emotibit private)
- crash core dump (emotibit private)
- hardware sensor-contellation profile (emotibit private)
Logging (class)
- setup log
- Debug log
- Crash log

Summary

There is a bit more to be thought through to identify how these classes (defined above) will interact with other classes in EmotiBit and the information exchange.
- The information exchange will dictate the structure of files
A high level discussion on this topic may also bring out other types not thought of yet.

Conclusions

We should shelf this PR till we define the above listed classes better.
- The pros for doing so
  - higher likelihood of not moving code again after we complete the PR
  - relatively low tax on task switching as this is a very defined code add.
  - It is very consolidated and well documented, so picking it up later should be relatively easy.
  - It is not an "essential" feature add.
- The cons for doing so
  - we will leave an open PR
  - This is a good feature add to have, and keep testing to gauge if it needs to change/evolve. (needs more time in master)

Next steps

discuss and define the structure mentioned above.
Define the above classes in more detail + information flow

…otiBitConfigManager

produceconsumerobot

@nitin710 code review complete

produceconsumerobot · 2024-03-14T16:39:55Z

EmotiBit.cpp

 	{
 		char input;
 		input = Serial.read();
-		if (input == 'R')


What's the reasoning for moving restartMcu() out of the first serial.available() block?
As I recall this was added to enable enterprise wifi compatibility. Are we sure moving it still covers that purpose?

I moved it to consolidate serial inputs to one location.
This shouldn't affect the enterprise wifi, because the reset option is still above the sensor setup and WiFi setup. But, I did not test it.

Thinking more about, there is no reason we have a reset option downstream in the code.

If I weigh the pros and cons of having promts together in code, vs possibly breaking enterprise, preserving enterprise makes more sense.

So, I will add it back to this place. Will add a comment on why its there.

produceconsumerobot · 2024-03-14T16:52:34Z

EmotiBit.cpp

+			if (Serial.available() >= 0)
+			{
+				char c = Serial.read();
+				if (c == 'C') 


It seems like we need 'C' (and perhaps the other serial inputs) to be defined as e.g. Inputs::CREDS in EmotiBitSerial.h so that we don't have magic chars littering the code

Fixed. I liked your namespace.
SO changed letters to EmotiBitSerial::Input::CRED_UPDATE

produceconsumerobot · 2024-03-14T16:58:50Z

EmotiBit.cpp

 			timeSinceLastPrint = millis();
+			if (configFileError)
+			{
+				Serial.println("Press \"C\" to add/update cofig file.");


Serial.prints should probably also refer to EmotiBitSerial Inputs::CREDS or similar to avoid magic chars lying around

produceconsumerobot · 2024-03-14T17:06:19Z

EmotiBit.cpp

-		Serial.println("{\"WifiCredentials\": [{\"ssid\":\"SSSS\",\"password\" : \"PPPP\"}]}");
-		// ToDo: verify if we need a separate case for FACTORY_TEST. We should always have a config file, since FACTORY TEST is a controlled environment
-		setupFailed("Config file not found");
+	if(loadConfig)


Is there a reason that this loadConfig block doesn't live inside EmotibitConfigManager?

Moved it to ConfigManager. Created a bit of a hassle, but its done!

produceconsumerobot · 2024-03-14T17:34:22Z

EmotiBitConfigManager.cpp

@@ -0,0 +1,287 @@
+#include <EmotiBitConfigManager.h>
+
+bool EmotiBitConfigManager::init(uint8_t sdCsPin)


I'm confused why we have/need to have SD.begin() inside EmotiBitConfigManager. It seems like makes it more spaghetti-like and less clear who's in charge of calling SD.begin and thus makes it more likely it getting called multiple times, which I'm not 100% sure what the behavior would be. Can we simply require sdInit() to be called within EmotiBit.cpp prior to calling updateWiFiCredentials?

Fixed. As discussed, EmotiBIt now inits SD and passes pointer to ConfigManager class

produceconsumerobot · 2024-03-14T19:31:06Z

EmotiBitConfigManager.cpp

+					file = SD.open(configFilename);
+					parseError = deserializeJson(configAsJson, file); 
+					file.close();
+				}


It seems like perhaps the code logic could be simplified if(!fileExists || parseError) was handled here by creating new file with zero creds?

Fixed the logic. Yes, i bubbled if(!fileExists || parseError) logic upstream. i think it's more readable now

EmotiBitConfigManager.cpp

produceconsumerobot · 2024-03-14T19:41:21Z

EmotiBitConfigManager.cpp

+								configAsJson["WifiCredentials"].remove(deleteIndex);
+								SD.remove(configFilename);
+#if defined ARDUINO_FEATHER_ESP32
+								file = SD.open(configFilename, FILE_WRITE);


It seems like this SD.open + serialJsonPretty is a copy/paste of code above with poorer error handling. Perhaps it should be pulled out int a function?

Yes, created a new function to handle file creation from JsonElement

EmotiBitConfigManager.cpp

produceconsumerobot · 2024-03-14T19:44:23Z

EmotiBitConfigManager.cpp

+						}
+						else
+						{
+							// send list of existing credentials


This comment seems out of place/perhaps vestigial.

Outdated comment. removing.

produceconsumerobot

@nitin710 testing review complete

produceconsumerobot · 2024-03-14T21:02:51Z

EmotiBitConfigManager.cpp

+	EmotiBitSerial::sendMessage(EmotiBitFactoryTest::TypeTag::FIRMWARE_VERSION, emotibitFwVersion);
+	// instructions
+	Serial.println("Options available:");
+	Serial.println("1. Add a wifi credential. Usage: @WA,{\"ssid\":\"SSSS\",\"password\" : \"PPPP\"}~");


I think perhaps it could be clearer to format the instructions as:

Add a wifi credential -- Send: @Wa,{"ssid":"SSSS","password" : "PPPP"}~

produceconsumerobot · 2024-03-14T21:03:47Z

EmotiBitConfigManager.cpp

+					if(configAsJson["WifiCredentials"].size())
+					{
+						Serial.println("##################################");
+						Serial.println("config file credetials:");


Add n to credetials

Capital c in Config

produceconsumerobot · 2024-03-14T21:04:25Z

EmotiBitConfigManager.cpp

+			}
+			else
+			{
+				Serial.println("not a valid serial message. Expecting @TYPETAG,PAYLOAD~");


Capital N in not

Add a : to Expecting: @TYPETAG,PAYLOAD~

produceconsumerobot · 2024-03-14T21:08:27Z

EmotiBitConfigManager.cpp

+	// Send EmotiBit Firmware version to host
+	EmotiBitSerial::sendMessage(EmotiBitFactoryTest::TypeTag::FIRMWARE_VERSION, emotibitFwVersion);
+	// instructions
+	Serial.println("Options available:");


Should the Options available be printed after every command so that you don't have to scroll back up to see the options?

I played with it, i personally didn't like it. Too much going on the screen.

nitin710 · 2024-03-22T02:46:08Z

@produceconsumerobot made the requested changes. Ready for review. (long list of changes.)

produceconsumerobot

It would probably be good if I test it one more time if you want to drop a bin file in the PR, but code looks much nicer.

produceconsumerobot · 2024-03-23T00:35:48Z

EmotiBitConfigManager.cpp

+#ifdef ARDUINO_FEATHER_ESP32
+bool EmotiBitConfigManager::init(fs::SDFS* sd)
+{
+	SD = sd;


I might have chosen a variable name that doesn't overload SD to avoid confusion, but I don't think it matters much.

Leaving it as it is.

produceconsumerobot · 2024-03-23T00:39:44Z

EmotiBitConfigManager.cpp

+	EmotiBitSerial::sendMessage(EmotiBitFactoryTest::TypeTag::FIRMWARE_VERSION, emotibitFwVersion);
+	// instructions
+	Serial.println("Options available:");
+	Serial.println("1. Add a wifi credential. Send: @WA,{\"ssid\":\"SSSS\",\"password\" : \"PPPP\"}~");


Come to think of it, we could probably use variables here instead of magic hard coded @ls~ etc, but it's not a huge deal since we're probably loathe to change these.

jaemindev and others added 4 commits August 31, 2023 14:43

Created setupfailed function for sd card and the config mode function…

f83f74d

… (incomplete).

Created function for parsing packet, currently empty with comments.

a437476

Added support for editing WiFi creds through serial monitor

263ab25

working add creds over serial

8e77eb0

nitin710 mentioned this pull request Sep 14, 2023

Introducing typetags to support WiFi creds over serial EmotiBit/EmotiBit_XPlat_Utils#25

Merged

7 tasks

nitin710 added 3 commits September 15, 2023 16:09

updated wifi cred serial function to add LS functionality

3c428cc

Added minor patch to serial output

3955fa5

todo comment

021f3d5

nitin710 mentioned this pull request Sep 26, 2023

Feat external board definition version control #284

Closed

7 tasks

Added updates to serial interface

b2fdffc

nitin710 mentioned this pull request Sep 26, 2023

Documentation on adding WiFi credentials through serial EmotiBit/EmotiBit_Docs#115

Merged

nitin710 added 2 commits March 8, 2024 15:29

moved wifi creds update onver serial out of emotibit class. Adding Em…

5a51e57

…otiBitConfigManager

minor serial print update

f950d5d

nitin710 requested a review from produceconsumerobot March 8, 2024 20:39

produceconsumerobot requested changes Mar 14, 2024

View reviewed changes

nitin710 added 6 commits March 21, 2024 15:35

updated logic of Config manager credential updater

791d75c

minor change

954f50f

fixed typos

a6eb8b0

fixed flow of control for SD init in ConfigManager

e36acf9

adding unit test for testing EmotiBit serial parsing

a158843

moved where config file is loaded. bug fix in emotibitWifi

bf5184d

produceconsumerobot reviewed Mar 23, 2024

View reviewed changes

version bump to 1.10.0. feature added- wifi creds over serial

6f3682d

nitin710 merged commit b4b8417 into master Mar 26, 2024

nitin710 deleted the feat-sdCardWiFiCredentials branch March 26, 2024 17:05

		@@ -0,0 +1,287 @@
		#include <EmotiBitConfigManager.h>

		bool EmotiBitConfigManager::init(uint8_t sdCsPin)

Conversation

nitin710 commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Requirements

Issues Referenced

Documentation update

Notes for Reviewer

Testing

Results

Feature Tests

Steps to test

Shared files

Checklist to allow merge

Uh oh!

nitin710 commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WIP

Uh oh!

nitin710 commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Moving this provision to a different class.

Current functions getting affected

New class

Uh oh!

nitin710 commented Sep 27, 2023

Discussion points

Uh oh!

nitin710 commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

possible stored data types

Organizing into categories

Summary

Conclusions

Next steps

Uh oh!

produceconsumerobot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

produceconsumerobot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nitin710 commented Sep 14, 2023 •

edited

Loading

nitin710 commented Sep 26, 2023 •

edited

Loading

nitin710 commented Sep 26, 2023 •

edited

Loading

nitin710 commented Oct 6, 2023 •

edited

Loading