EmotiBit / EmotiBit_FeatherWing

Firmware to run Emotibit with Adafruit feather M0 WiFi and Feather ESP32
MIT License
19 stars 8 forks source link

Updated from ArduinoJson 5 to latest. #278

Closed JaeminBBQ closed 1 year ago

JaeminBBQ commented 1 year ago

Description

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

Requirements

Issues Referenced

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.

Notes for Reviewer

Testing

Results

Test to make sure config.txt file is still being parsed as expected. Parse Results Comments
Screenshot 2023-06-15 at 11 25 04 AM 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
Screenshot 2023-06-15 at 11 54 00 AM The only difference is in char 263, line 1
Screenshot 2023-06-15 at 11 54 42 AM The difference is the firmware version
Screenshot 2023-06-15 at 11 56 57 AM 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
Free_Memory_Old Free_Memory_new

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

Checklist to allow merge

nitin710 commented 1 year ago

Review 01

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

Some notes about PR organization:

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

JaeminBBQ commented 1 year ago

Review Checklist

nitin710 commented 1 year ago

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

  1. ✔️ 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"
    }
    ]
    }

    image

  2. ✔️ Differences between info.json files

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

@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.