Updated from ArduinoJson 5 to latest. #278

JaeminBBQ · 2023-06-08T23:38:55Z

Description

Updating the ArduinoJson library to the latest version and added free_memory() to display during bootup.

Requirements

ArduinoJson version 6

Issues Referenced

Fixes Upgrade usage of ArduinoJson to support latest version #31

Documentation update

Documentation should be changed under:
Keeping EmotiBit up-to date => Update firmware using Arduino IDE => Library List
where "ArduinoJson (version 5.13.5, not v6.x.x)" should be replaced with "ArduinoJson" since it is the latest version.

PR to documentation change.

Updated ArduinoJson version in library list. EmotiBit_Docs#113

Notes for Reviewer

Json update #39 (Existing PR on upgrade)
The ArduinoJson Library must be updated to 6, the changes will not work with version 5.
The free memory function was added to the function where emotibit data is displayed. This was only added in the case a featherwing m0 is used.

Testing

Results

Test to make sure config.txt file is still being parsed as expected.

Parse Results	Comments
	The config.txt is parsed correctly but missing values are stored as a string "null" rather than an actual null value. This get around this, an if statement is used to set "null" strings to "".

Test to make sure the info.json file created when a recording is started is the same as the one created with the currect release.

Info.json diff results	Comments
	The only difference is in char 263, line 1
	The difference is the firmware version
	Previous firmware version

The library update reduces the memory used by the EmotiBit which was checked using the free memory function as seen here.

Memory Before	Memory After

Feature Tests

Add the test heading from "EmotiBit Feature Test Protocol" here.

Steps to test

Import the steps from the EmotiBit Feature Test Protocol for quick access for the reviewer

Shared files

Firmware binary: 1.8.1.fix-JsonLibUpdate.1.zip
Other files.

Checklist to allow merge

…during bootup.

EmotiBit.cpp

JaeminBBQ · 2023-06-09T21:28:52Z

EmotiBit.cpp

 		Serial.print("Adding SSID: ");
-		Serial.print(ssid); 
+		Serial.print(ssid);
+


Solution to "null" being returned instead of empty string.

If there a reason why we have a if block instead of using the | (OR) operation as indicated in the example? (also used in the existing code)

When config.txt is parsed, missing values are stored as a string "null" rather than an actual null value. To get around this, an if statement is used to set "null" strings to "". I didn't check what was returned before the update but the string "null" began to appear after updating to the latest version.

I removed the "if" block. Looked a bit redundant. Instead, now we use the "OR" functionality as demonstrated in the example https://arduinojson.org/v6/api/jsonvariant/or/

nitin710 · 2023-06-15T12:11:18Z

Review 01

Pretty straight forward. Please see my comments below (some process changes, some code changes and comments on testing results).

I see that the version was not bumped in this FW branch. When making a fix/improvement, we usually change the firmware by adding a identifier. The rules to create this identifier are listed in the CFL standard github workflow. Currently, if someone installs the linked firmware binary, to them, there will be no difference between this "still in dev firmware" to the one linked in the latest release because they are both v1.8.1. This can be quite dangerous since we don't yet have complete traceability of the dependencies.
- This may have been a miss-communication since the doc (pointed above) does not have an explicit example.
- The usual flow is to change the FW version which can be found here
  
  EmotiBit_FeatherWing/EmotiBit.h
  
  Line 53 in 9705ef7
  
  String firmware_version = "1.8.1";
- Can you make this change and also update the linked binaries? Let make sure we don't spread different firmwares with same version!
I think there should be 2 more tests to be run
- Test to make sure cofig.txt file is still being parsed as expected. (I think this is implicit since you tested streaming, but lets make it explicit here)
- Test to make sure the info.json file created when a recording is started is the same as the one created with the currect release.
  - I think the steps are
    - capture recording with current latest release (v1.8.1)
    - capture recording with this fw patch
    - compare the info.json files
      - You may consider using a diff tool like kdiff3 to make your life easier in the comparison.
  - We are expecting everything to be the same except the firmware version in the files.
  - Please update the PR with the results.
Can you also make the required documentation changes in the docs repo? I think the steps are
- Create a branch (in EmotiBit_docs), make the changes
- Create a PR.
- Link that PR here.
- RE the change, lets state it explicitly which version of ArduinoJSON we are using. This will prevent any support issues if ArduinoJSON makes a breaking change.
Updated from ArduinoJson 5 to latest. #278 (comment)

Some notes about PR organization:

Just a semantic thing, but Added free_memory() to display during bootup is currently floating in the PR description. Probably nesting it under an appropriate heading is better. Remember, the description is the first thing a reviewer (or anyone else) will read when coming to this PR. making it super clear what the PR does will help avoid confusion.
In the Results section, its better to have a table or a itemized list to highlight the tests run. Lets conform to this requirement and structure the results section accordingly.
- Also, move the memory test details into the testing results section.
The Feature Tests section should hold the test headings to any tests linked from the EmotiBit Feature test protocol doc. If you did not add any feature tests, then it is OK to leave that section blank.
If you want the reviewer to test something, Adding some instructions in the "steps to test" section can be helpful.

I have not personally run the code yet. I will wait to test it after you make the changes (at least till you bump the firmware version).

Also, please feel free to add/update examples to any docs you may have referenced to create this PR. For example, adding an example about how to change the firmware version in the CFL standard github workflow will be helpful!

Review complete. @JaeminBBQ

EmotiBit.h

JaeminBBQ · 2023-06-16T16:38:56Z

Can you make this change and also update the linked binaries? Let make sure we don't spread different firmwares with same version!
Test to make sure cofig.txt file is still being parsed as expected. (I think this is implicit since you tested streaming, but lets make it explicit here)
Test to make sure the info.json file created when a recording is started is the same as the one created with the currect release.
Can you also make the required documentation changes in the docs repo? I think the steps are
Updated from ArduinoJson 5 to latest. #278 (comment)

Review Checklist

Just a semantic thing, but Added free_memory() to display during bootup is currently floating in the PR description. Probably nesting it under an appropriate heading is better. Remember, the description is the first thing a reviewer (or anyone else) will read when coming to this PR. making it super clear what the PR does will help avoid confusion.
In the Results section, its better to have a table or a itemized list to highlight the tests run. Lets conform to this requirement and structure the results section accordingly.
Also, move the memory test details into the testing results section.

nitin710 · 2023-06-16T20:20:42Z

Changed implementation for handling non-existent field in config file. Link to comment: https://github.com/EmotiBit/EmotiBit_FeatherWing/pull/278/files#r1232736934

Tests run

✔️ incomplete/wrong config file handling (missing SSID field changed to "")
For a config file with contents

{
  "WifiCredentials": [
    {
      "ssid": "brainwaves2.4",
      "password": "brainwaves"
    },
    {
      "password": "emotibitRocks" // notice missing SSID field
    },
    {
      "ssid": "redacted",
      "password": "redacted"
    }
  ]
}

✔️ Differences between info.json files

raw info files.zip
FW version was changed
Filename was changed
floating point value of the EDA constants seem to be different, but its not a huge deal

nitin710 · 2023-06-16T20:41:20Z

@JaeminBBQ I made some minor changes in the code, take a look at this comment.
I also tested it working for config files and info files.
Looks good!

Let merge to master. Remember to complete the FW merge checklist in the PR description ☝🏽 .Make sure to pull before you make the changes to avoid conflicts.

Updated from ArduinoJson 5 to latest. Added free_memory() to display …

a180879

…during bootup.

JaeminBBQ added enhancement New feature or request Priority-Medium labels Jun 8, 2023

JaeminBBQ commented Jun 9, 2023

View reviewed changes

EmotiBit.cpp Show resolved Hide resolved

JaeminBBQ commented Jun 9, 2023

View reviewed changes

JaeminBBQ requested a review from nitin710 June 9, 2023 22:13

Updated Firmware Version.

a3d7130

JaeminBBQ commented Jun 15, 2023

View reviewed changes

EmotiBit.h Outdated Show resolved Hide resolved

jaemindev added 2 commits June 15, 2023 11:27

Fixed inconsistent config file parse.

601f43f

Renamed Firmware version.

cca8ab3

removed if-block for checking null contents in config file

35033a9

jaemindev added 3 commits June 18, 2023 19:57

Updated Version on EmotiBit.h and library.properties

fdd66d2

Updated version from 2.0.0 to 1.9.1.

93bede0

Fixed wrong version number 1.9.1 to 1.9.0.

87ad8e5

JaeminBBQ merged commit e2ed2dc into master Jun 19, 2023

JaeminBBQ deleted the fix-JsonLibUpdate branch June 19, 2023 18:43

JaeminBBQ mentioned this pull request Jun 19, 2023

Json update #39

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated from ArduinoJson 5 to latest. #278

Updated from ArduinoJson 5 to latest. #278

JaeminBBQ commented Jun 8, 2023 •

edited

Loading

JaeminBBQ Jun 9, 2023

nitin710 Jun 15, 2023

JaeminBBQ Jun 16, 2023

nitin710 Jun 16, 2023

nitin710 commented Jun 15, 2023

JaeminBBQ commented Jun 16, 2023 •

edited by nitin710

Loading

nitin710 commented Jun 16, 2023 •

edited

Loading

nitin710 commented Jun 16, 2023 •

edited

Loading

Updated from ArduinoJson 5 to latest. #278

Updated from ArduinoJson 5 to latest. #278

Conversation

JaeminBBQ commented Jun 8, 2023 • edited Loading

Description

Requirements

Issues Referenced

Documentation update

Notes for Reviewer

Testing

Results

Feature Tests

Steps to test

Shared files

Checklist to allow merge

JaeminBBQ Jun 9, 2023

Choose a reason for hiding this comment

nitin710 Jun 15, 2023

Choose a reason for hiding this comment

JaeminBBQ Jun 16, 2023

Choose a reason for hiding this comment

nitin710 Jun 16, 2023

Choose a reason for hiding this comment

nitin710 commented Jun 15, 2023

Review 01

JaeminBBQ commented Jun 16, 2023 • edited by nitin710 Loading

Review Checklist

nitin710 commented Jun 16, 2023 • edited Loading

Tests run

nitin710 commented Jun 16, 2023 • edited Loading

JaeminBBQ commented Jun 8, 2023 •

edited

Loading

JaeminBBQ commented Jun 16, 2023 •

edited by nitin710

Loading

nitin710 commented Jun 16, 2023 •

edited

Loading

nitin710 commented Jun 16, 2023 •

edited

Loading