Refactored testing, Deprecated OF dependency in EmotiBit packet by Joseph-Jacobson · Pull Request #28 · EmotiBit/EmotiBit_XPlat_Utils

Joseph-Jacobson · 2025-05-29T15:15:48Z

Description

The purpose of this PR is to deprecate OF dependencies in EmotiBit Packet, and refactor appendTestData so it will have expected results that can be compared from the EmotiBit and an external source. Below is a more detailed list of the additions.

Moved and refactored testing logic from for appendTestData in EmotiBit.cpp to EmotiBitPacket
Added structure to use CMake to create an exe from a testing file that creates a comparable output file
BASH script to compare SD card output and main tester
Removed OF dependancies
Bumped versioning to FeatherWing 1.13.1 and XPlat 1.7.1
Added a new createPacket that intakes a header and data

Requirements

To you run this branch you would need a minimum of:

FeatherWing 1.13.1
XPlat 1.7.1

Issues Referenced

Issue #29

Documentation update

None

Notes for Reviewer

Requirements for review

CMake
Git Bash
Platform.io
EmotiBit Oscilloscope
1.12.1 AppendTestData on the Featherwing branch
An EmotiBit that is configured to your WiFi

Additional Review Information

New testing structure

Added CMake file: This is used to make main.cpp.
Added main.cpp: This is used to run tests and outputs test.csv
Added BASH script: This is used to run our tests by comparing the test.csv with an expected output
New file structure for testing

OF deprecation

Moved ArduinoString.h: Moved into XPlat from ofx
Edited EmotiBitPacket.h and EmotiBitPacket.cpp: Refactored OF out from being a dependency. I created a function to input ofGetElapsedTimeMillis into the headers if we are i nan OF build. headerToString now uses to_string. Created three test functions: createTestDataPacket, used as an entry point to access each test and iterates for the specified number. It also creates the packets using the new createTestPacket. createTestSawtoothData is used to generate the sawtooth data packets. testHeaderToString is used to test the OF refactor. These two functionalities have determined outputs.
Added a new createPacket that intakes a header of class EmotiBitPacket::Header and a template for data. This allows users to pass in either a String or std::string for data. Diagram below:

Testing

Please follow "Mock Data Testing" and "Arduino String Testing" in the "EmotiBit Feature Test Protocols" document

To run this test you will need to pull both this, and 1.12.1 AppendTestData from the featherwing branch, and ofX. This process was built using Platform.io and the EmotiBit Oscilloscope. I also ran the follow tests on an Arduino build, and the oscilliscope with the new xPlat.

Results

Note: for CI both tests return 0/1 for pass/fail. The terminal prints is to communicate more information if needed.

For MockDataTest: the BASH script will show the results between the new MockDataTest and the EmotiBit's sd card result. The file path to the sd card will need to be specified.

Success:
Fail:

For ArduinoStringTest: the BASH script will show the result of the pass/fails.

Success:
Fail:

Feature Tests

none

Shared files

Firmware binary: [Link to firmware binary]
None
Other files.
None

Checklist to allow merge

Screenshots:

added OF tag, cleaned up bash script

Copilot

Pull Request Overview

This PR removes openFrameworks dependencies, refactors packet creation for deterministic testing, and adds tooling to build and compare test data outputs.

Refactor EmotiBitPacket to remove OF-specific code and introduce appendTestDataMessage, createTestSawtoothData, and testHeaderToString methods
Add EmotiBitDataTester executable with CMake support to generate a CSV of test packets
Provide a Bash script to locate and compare SD card output against the generated test CSV

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/EmotiBitPacket.h	Declared `maxTestLength` and new test data methods
src/EmotiBitPacket.cpp	Defined `maxTestLength`, refactored `headerToString`/`createPacket`, and implemented test data functions
src/EmotiBitDataTester.cpp	New CLI tool to call `appendTestDataMessage` and write `test.csv`
src/ArduinoString.h	Introduced a minimal `EmotiBit::String` wrapper around `std::string`
bashScripts/compare_test.sh	Script to find the most recent SD card CSV and diff against `test.csv`
CMakeLists.txt	Build configuration for `EmotiBitDataTester`

src/EmotiBitPacket.cpp

bashScripts/compare_test.sh

src/ArduinoString.h

minor edits to EmotiBitPackets and the bash script, added toDos,

nitin710

Review-01 by Nitin

I like that you took the time to populate the PR description. It reads well, but there are some issues that I have highlighted below.

General comments on the PR description

"Moved testing logic for appendTestData to EmotiBitPacket". Maybe having a link to where it was moved from can add context to the PR.
"Included CMake for running main testing file and associated data testing file". I am not sure it is clear enough to add value to the description. It either needs to in reviewer notes under a section "usage" or better explained if a part of the description.
I think the list under "requirements" better lives under Testing > Requirements.
- This section was intended more for "you need this in your stack to be able to compile/run the new features". For us, that would be FW/SW version x.y.z. We can discuss more on this.
Myabe adding a block diagram that explains the testing process helps in understanding the testing flow.
We should discuss branch naming, XPlat was at version 1.6.1. So i think the feature is built on top of that version. I believe, currently it is using the FW version as base.

File/folder structure

Generally, whenever we are adding new files/folders, there is always a question of naming conventions and location of the new files
I am not sure bashScripts is the correct name or the correct location.
- Are we using camel case as a convention? Why not bash-scripts or bash_scripts?
- It would also be nice to add the reasoning for the name. for example, could we should call it scripts, or maybe helper-scripts?
- regarding structure, could we have the following? (see the next bullet point for an alternate organization)

scripts
├── script1_name
│   ├── script1.sh
│   └── README.md
└── script2_name
    ├── script2.sh
    └── README.md

CMakeLists is another file i dont think belongs in root.
- I think it should live in a different folder. Maybe apps?
- There should be a correlation between this CMakeListt and the bashScript
- Maybe something like this is what we need?

apps (probably some other appropriate name?)
├── DataTester
│   ├── README.md
│   ├── CMakeLists.txt
│   ├── src
│   │   └── EmotiBitDataTester.cpp 
│   ├── scripts
│   │  ├── README.md 
│   │  └── script1.sh
|   └── build

ToDo

Bump the version in the library.properties files in root. Follow semantic versioning for the version bump

Reviewer notes

successfully compiled on PIO. Failed on Arduino, but i suspect it's a personal arduino setup issue.
Did not run the CMake or bash. If we move the files, the sript and possible CMake file will change. So, I plan to test it then,

CMakeLists.txt

src/ArduinoString.h

src/EmotiBitPacket.h

src/EmotiBitDataTester.cpp

bashScripts/compare_test.sh

src/EmotiBitPacket.h

1. bumped library 2. code refactor 3. file restructure for testing 4. added ArduinoString.h tests

produceconsumerobot

Overall there are a number of good things in the code changes, but there are a handful of places where the rationale for the choices weren't immediately clear to me. The biggest omission from the PR itself is that you should include your complete results that fully demonstrates your code tested working. I shouldn't have to run your code to be sure that it works. Similar to presentation of scientific results in a journal article, I should be able to read your thorough description of your methods and your results and have everything I need to assess whether your conclusions are supported by your methods and results.

tests/ArduinoStringTest/README.md

tests/ArduinoStringTest/src/main.cpp

tests/MockDataTest/scripts/run_test.sh

produceconsumerobot · 2025-06-13T00:42:11Z

src/EmotiBitPacket.cpp

 #else
 string EmotiBitPacket::headerToString(const Header &header)
 {
+	//Refactored to use to_string instead of ofToString


Did you confirm that to_string performs the same as ofToString for all pertinent data types?
I'm not sure if I'm tracking where that's been tested explicitly.

createTestHeader is used to test this refactor. We know to_string works the same as ofToString with the exceptions being it doesn't allow for string to string, which is why I omitted any conversion for the type tag. The other difference is to_string has more robust error handling.

src/EmotiBitPacket.cpp

nitin710

Review02 by Nitin

src/EmotiBitPacket.cpp

nitin710 · 2025-06-13T16:17:09Z

tests/ArduinoStringTest/CMakeLists.txt

+
+add_executable(ArduinoStringTest
+    src/main.cpp
+    ../../src/ArduinoString.h


maybe use target_include_directories function? I thnk add_executable should take cpp files by standard.

Is this meant to replace add_executable?

No, I *think*, add_executable is used to link src files to an executable.
_target_include_directories is used for specifying header files for the specified source files.

Gotcha, target_include_directories specifies the directory so I will be specifying src so we can access arduino string

tests/ArduinoStringTest/src/main.cpp

tests/MockDataTest/README.md

tests/MockDataTest/scripts/run_test.sh

nitin710 · 2025-06-13T16:29:57Z

tests/MockDataTest/src/main.cpp

+    uint16_t packetNumber = 0;
+
+    // Define the type tags for the test data, can be used to expand the test data in the future
+    const char* typeTags[] = {


Im not sure I understand why we are defining this here. Can you add your explanation on this comment thread?

I was defining it there as a method to access the type tags in a previous version, this was left in however can be easily removed.

Updated readmes, doxygen comments, and tests based on comments Cleaned up code Added new createPacket and deprecated old createPackets

nitin710

Left some minor feedback, but I think it looks good!
Did not run any tests.
I think, lets change the ArduinoStringTest script per my notes, and then we can merge it.

src/EmotiBitPacket.h

nitin710 · 2025-06-20T16:02:19Z

tests/ArduinoStringTest/CMakeLists.txt

+
+add_executable(ArduinoStringTest
+    src/main.cpp
+    ../../src/ArduinoString.h


No, I *think*, add_executable is used to link src files to an executable.
_target_include_directories is used for specifying header files for the specified source files.

tests/ArduinoStringTest/scripts/run_test.sh

produceconsumerobot

Looks good. 2 small comments.

src/EmotiBitPacket.cpp

produceconsumerobot · 2025-06-20T23:48:06Z

src/EmotiBitPacket.h

+	//! @param data Template parameter T for the data to include in the packet
+	//! @return String representation of the packet
+	template <typename T>
+	static String createPacket(const Header& header, const T& data)


I think output of Type T from createPacket() would make the function more versatile

Agreed, I had to add another conversion to ArduinoString.h to convert a String to std::string for c++ to work with this template T. Messy internally but now we don't need to expect String as the return type. Will need to test this function as well.

nitin710

Let's merge it!

After you merge to master, you can go ahead and make a release using the guidelines here: https://github.com/EmotiBit/.github/blob/master/Docs/FirmwareDependencyRelease.md

nitin710 · 2025-06-24T13:44:24Z

tests/CreatePacketTest/scripts/run_script.sh

+fi
+
+"$EXECUTABLE"
+RESULT=$?


I hope you also tested that "a fail" is registered as a fail by this script.
Trying to tweak the test to intentionally fail temporarily is a good way to make sure your "fail catch" is working!

I confirmed fails get caught by the script as well!

Joseph-Jacobson added 2 commits May 28, 2025 13:00

XPlat edits for PR

4c18a91

Minor edits

ed3a877

added OF tag, cleaned up bash script

Joseph-Jacobson mentioned this pull request May 30, 2025

refactored appendTestData EmotiBit/EmotiBit_FeatherWing#327

Merged

12 tasks

Joseph-Jacobson requested a review from Copilot May 30, 2025 18:58

Copilot AI reviewed May 30, 2025

View reviewed changes

src/EmotiBitPacket.cpp Outdated Show resolved Hide resolved

src/EmotiBitPacket.cpp Outdated Show resolved Hide resolved

src/EmotiBitPacket.cpp Show resolved Hide resolved

bashScripts/compare_test.sh Outdated Show resolved Hide resolved

src/ArduinoString.h Show resolved Hide resolved

ToDos

f36332a

minor edits to EmotiBitPackets and the bash script, added toDos,

Joseph-Jacobson mentioned this pull request May 30, 2025

Removing ArduinoString EmotiBit/ofxEmotiBit#270

Merged

12 tasks

Joseph-Jacobson requested review from nitin710 and produceconsumerobot May 30, 2025 20:17

nitin710 requested changes Jun 2, 2025

View reviewed changes

PR review changes

393b611

1. bumped library 2. code refactor 3. file restructure for testing 4. added ArduinoString.h tests

produceconsumerobot requested changes Jun 13, 2025

View reviewed changes

nitin710 requested changes Jun 13, 2025

View reviewed changes

Joseph-Jacobson added 3 commits June 18, 2025 10:25

Updates from PR and new createPacket

6e43bec

Updated readmes, doxygen comments, and tests based on comments Cleaned up code Added new createPacket and deprecated old createPackets

Fixed bug in ArduinoStringTest

766075f

Minor style changes

ef19319

Joseph-Jacobson requested review from nitin710 and produceconsumerobot June 18, 2025 19:15

Joseph-Jacobson changed the title ~~Refactored for defined testing~~ Refactored testing, Deprecated OF Jun 20, 2025

nitin710 requested changes Jun 20, 2025

View reviewed changes

Joseph-Jacobson added 2 commits June 20, 2025 10:51

Minor edits to arduino string test and deprecated arduino createPacket

226ac4c

Updated #endif in Packet.h

61ba575

produceconsumerobot requested changes Jun 20, 2025

View reviewed changes

nitin710 changed the title ~~Refactored testing, Deprecated OF~~ Refactored testing, Deprecated OF dependency in EmotiBit packet Jun 23, 2025

Added a test for the new createPacket

d6652ea

Joseph-Jacobson requested a review from nitin710 June 23, 2025 19:06

nitin710 approved these changes Jun 24, 2025

View reviewed changes

Renamed CreatePacketTest script to the correct name

2e1ed02

Joseph-Jacobson merged commit b7ce552 into master Jun 24, 2025

Conversation

Joseph-Jacobson commented May 29, 2025 • 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

Requirements for review

Additional Review Information

New testing structure

OF deprecation

Testing

Results

For MockDataTest: the BASH script will show the results between the new MockDataTest and the EmotiBit's sd card result. The file path to the sd card will need to be specified.

For ArduinoStringTest: the BASH script will show the result of the pass/fails.

Feature Tests

Shared files

Checklist to allow merge

Screenshots:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nitin710 left a comment

Choose a reason for hiding this comment

Review-01 by Nitin

General comments on the PR description

File/folder structure

ToDo

Reviewer notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

produceconsumerobot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Joseph-Jacobson Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nitin710 left a comment

Choose a reason for hiding this comment

Review02 by Nitin

Uh oh!

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!

Joseph-Jacobson commented May 29, 2025 •

edited

Loading

Joseph-Jacobson Jun 16, 2025 •

edited

Loading