Solids Test #348

zizz-0 · 2025-12-09T01:50:29Z

Description

Code to test solids motors using Grim Reefer

Measure ADC data in 10 second test runs
Store test data (up to 30 tests) in flash storage
Dump either specific test number or all tests to serial
Trigger tests either using terminal command or meep
Set test calibration names

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Terminal test commands tested on Grim Reefer using minicom. Solids members tested motors using meep connected to TX. Key switch functionality has only been tested by pulling RX, not with an actual key switch

Checklist:

New functionality is documented in the necessary spots (i.e new functions documented in the header)
The CI checks are passing
I reviewed my own code in the GitHub diff and am sure that each change is intentional
I feel comfortable about this code flying in a rocket

…h-Initiative/FSW into zizz/solids-test-board

buzzer accidentally auto formatted (sorrry)

…h-Initiative/FSW into zizz/solids-test-board

…ing. test read one implemented 🎷🐛

…d of 250

…h-Initiative/FSW into zizz/solids-test-board

… long beep)

cowsed · 2025-12-09T02:02:56Z

app/other/solids_test/sample.yaml

@@ -0,0 +1,9 @@
+sample:
+  description:
+  name: grim_reefer


can you change the name to solids board here

app/other/solids_test/boards/grim_reefer.overlay

app/other/solids_test/include/adc_reading.h

cowsed · 2025-12-20T17:14:00Z

app/other/solids_test/include/adc_reading.h

+/**
+ * @brief Waits for ADC reading event to start, then reads ADC samples for 10 seconds
+ */
+void adc_reading_task();


what i failed to say in the other comment was that adc_reading_task need not be exposed. i think this can be safely kep private in adc_reading.c

after further pondering and the comments about the SYS_INIT. do not follow the comment above. i think this should be exposed so that all the K_THREAD_DEFINEs can be in main. so main looks something like

#include<a bunch of thigns.h> SYS_INIT(adc_init) SYS_INIT(buzzer_init) ... K_THREAD_DEFINE(adc_reading_task) K_THREAD_DEFINE(storage_task) K_THREAD_DEFINE(buzzer_task)

kinda for the same reason you mentioned for sys_inits - its good to see them all in one place

and then if you do that it would be nice to put a comment on *_init and *_task saying like these init and run the subsystem. only call them once and all that jazz

AarC10 · 2025-12-20T17:08:37Z

app/other/solids_test/boards/grim_reefer.overlay

+        address = <1>;
+        analog-clock-prescaler = <0>;
+        boost-current-bias = <0>;
+        spi-max-frequency = <DT_FREQ_M(18)>;


Checking the datasheet, MCP3561 is capable of 20 mhz

AarC10 · 2025-12-20T17:09:47Z

app/other/solids_test/include/adc_reading.h

+
+/**
+ * @brief Read one ADC sample
+ * @param adc_val Pointer to value where sample will be written


Can you please add [in] and [out] for the @params in your function documentation

AarC10 · 2025-12-20T17:12:42Z

app/other/solids_test/src/adc_reading.c

+    }
+}
+
+void adc_reading_task() {


Pretty sure you should have a (void*, void*, void*) here to avoid any warnings you get for declaring this a task.

AarC10 · 2025-12-20T17:14:45Z

app/other/solids_test/prj.conf

+CONFIG_ADC=y
+CONFIG_ADC_LOG_LEVEL_ERR=y # Will spam annoying error at wrn level
+
+CONFIG_ADC_MCP356XR=y # new driver: https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/adc/Kconfig.mcp356xr


Unnecessary comment

AarC10 · 2025-12-20T17:17:12Z

app/other/solids_test/src/buzzer.c

+    for (int i = 0; i < 10; i++) {
+        printk("BEEP ");
+        set_buzz(1);
+        k_msleep(1000);
+        set_buzz(0);
+        k_msleep(1000);
+    }
+    printk("\n");


Why no logger here?

AarC10 · 2025-12-20T17:18:34Z

app/other/solids_test/src/buzzer.c

+        printk("BEEP ");
+        set_buzz(1);
+        k_msleep(100);
+        set_buzz(0);
+        k_msleep(100);
+    }
+    printk("\n");
+}
+
+void test_end_beep() {
+    test_running = false;
+    printk("BEEEEEP\n");
+    set_buzz(1);
+    k_msleep(100);
+    set_buzz(0);
+    k_msleep(100);
+    set_buzz(1);
+    k_msleep(1000);
+    set_buzz(0);
+}
+
+void continuous_beep() {
+    printk("BEEEEEEEEEEEEEEEEEEEEEEEP\n");


Or here lol. Maybe also be a little more descriptive than just adding more Es, so people who didn't write the code have a better idea of whats going on. Or just remove the prints if these are debug prints

hater (they were just debug prints since I had the buzzer disabled while testing, will remove)

AarC10 · 2025-12-20T17:30:04Z

app/other/solids_test/src/button.c

+}
+
+void buzzer_task() {
+    while (1) {


Keep it consistent and have a while (true) here

Copilot

Pull request overview

This PR adds new functionality to test solids motors using the Grim Reefer board. The implementation collects ADC data during 10-second test runs, stores up to 30 tests in flash memory with calibration metadata, and supports triggering tests via both terminal commands and external hardware (meep) with appropriate safety indicators.

Key changes:

ADC data collection system with 1ms sample rate for 10-second test duration
Flash storage management supporting 30 test runs with calibration names and test metadata
Dual trigger modes: terminal commands and hardware button/key switch with buzzer feedback
Shell command interface for test control, data dumping, and flash management

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
`app/other/solids_test/src/main.c`	Application entry point with SYS_INIT configuration for ADC, buzzer, and button modules
`app/other/solids_test/src/shell_cmds.c`	Shell command interface for test control (start, stop, dump, erase, ematch)
`app/other/solids_test/src/control.c`	Central control logic coordinating ADC reading and flash storage operations
`app/other/solids_test/src/flash_storage.c`	Flash storage thread managing test data persistence with metadata
`app/other/solids_test/src/adc_reading.c`	ADC sampling thread with timer-based collection and automatic 10-second duration
`app/other/solids_test/src/buzzer.c`	GPIO control for buzzer, LDO, and ematch pins with beep notification functions
`app/other/solids_test/src/button.c`	GPIO interrupt handlers for button (TX) and key switch (RX) with buzzer task
`app/other/solids_test/include/*.h`	Header files with function documentation and struct definitions
`app/other/solids_test/boards/grim_reefer.overlay`	Device tree configuration for ADC, GPIO pins, and SPI peripherals
`app/other/solids_test/prj.conf`	Zephyr build configuration enabling ADC, flash, shell, and GPIO support
`app/other/solids_test/CMakeLists.txt`	Build system configuration
`app/other/solids_test/Kconfig`	Kconfig integration
`app/other/solids_test/sample.yaml`	Test sample configuration for CI

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-20T17:34:35Z

app/other/solids_test/src/flash_storage.c

+    flash_read(flash_dev, block_addr, calib_name, sizeof(calib_name));
+    block_addr += 32;
+
+    flash_read(flash_dev, block_addr, test_type, sizeof(test_type));
+    block_addr += sizeof(test_type);
+
+    shell_print(shell, "================================\nDumping Test #%d", test_index);
+    shell_print(shell, "CALIBRATION: %s", calib_name);
+    shell_print(shell, "Test triggered by %s", test_type);


The global variable 'test_type' is read in flash_dump_one without any protection against concurrent modification. If a new test starts while flash_dump_one is executing, the test_type being displayed could be inconsistent with the actual test data being read from flash.

Suggested change

flash_read(flash_dev, block_addr, calib_name, sizeof(calib_name));

block_addr += 32;

flash_read(flash_dev, block_addr, test_type, sizeof(test_type));

block_addr += sizeof(test_type);

shell_print(shell, "================================\nDumping Test #%d", test_index);

shell_print(shell, "CALIBRATION: %s", calib_name);

shell_print(shell, "Test triggered by %s", test_type);

char local_test_type[sizeof(test_type)];

flash_read(flash_dev, block_addr, calib_name, sizeof(calib_name));

block_addr += sizeof(calib_name);

flash_read(flash_dev, block_addr, local_test_type, sizeof(local_test_type));

block_addr += sizeof(local_test_type);

shell_print(shell, "================================\nDumping Test #%d", test_index);

shell_print(shell, "CALIBRATION: %s", calib_name);

shell_print(shell, "Test triggered by %s", local_test_type);

Copilot · 2025-12-20T17:34:36Z

app/other/solids_test/src/buzzer.c

+        LOG_ERR("Failed to conf ldo enable pin :(");
+        return -1;
+    }
+


The ematch GPIO is configured but never checked for successful initialization. If gpio_pin_configure_dt fails for the ematch pin, the function returns 0 (success) without configuring this critical safety component. This should include error checking and return an error if ematch configuration fails.

Suggested change

ret = gpio_pin_configure_dt(&ematch, GPIO_OUTPUT_INACTIVE);

if (ret < 0) {

LOG_ERR("Failed to conf ematch pin :(");

return -1;

}

Copilot · 2025-12-20T17:34:36Z

app/other/solids_test/src/flash_storage.c

+
+int flash_dump_one(const struct shell *shell, uint32_t test_index) {
+    if (test_index >= MAX_TESTS ){
+        shell_print(shell, "Pick a valid test [0-29]");


The error message says "Pick a valid test [0-29]" but uses a hardcoded range. If MAX_TESTS is changed from 30, this error message will become incorrect. Consider using a format string that incorporates MAX_TESTS dynamically, such as "Pick a valid test [0-%d]", MAX_TESTS - 1.

Suggested change

shell_print(shell, "Pick a valid test [0-29]");

shell_print(shell, "Pick a valid test [0-%u]", MAX_TESTS - 1);

Copilot · 2025-12-20T17:34:36Z

app/other/solids_test/src/flash_storage.c

+
+K_THREAD_DEFINE(storage_thread, 2048, flash_storage_thread_entry, NULL, NULL, NULL, STORAGE_THREAD_PRIORITY, 0, 1000);
+
+static char calibration[32];


The magic number 32 is used for the calibration name buffer size in multiple places without being defined as a constant. This appears on lines 18, 135-136, 188, 190, 224-226, and in control.c line 17. Consider defining this as a named constant (e.g., CALIB_NAME_MAX_LEN) in config.h to improve maintainability and ensure consistency.

Copilot · 2025-12-20T17:34:36Z

app/other/solids_test/src/buzzer.c

+void test_start_beep() {
+    test_running = true;
+    for (int i = 0; i < 3; i++) {
+        printk("BEEP ");
+        set_buzz(1);
+        k_msleep(100);
+        set_buzz(0);
+        k_msleep(100);
+    }
+    printk("\n");
+}
+
+void test_end_beep() {
+    test_running = false;


The test_running flag is set to true in test_start_beep and false in test_end_beep, which is misleading. These are beep notification functions, not test control functions. Setting test state within buzzer notification functions creates confusing control flow and makes it unclear who owns the test_running state.

Copilot · 2025-12-20T17:34:37Z

app/other/solids_test/src/buzzer.c

+static const struct gpio_dt_spec buzzer = GPIO_DT_SPEC_GET(DT_ALIAS(buzzer), gpios);
+static const struct gpio_dt_spec ematch = GPIO_DT_SPEC_GET(CAM_EN_NODE, gpios);
+
+static bool test_running = false;


The variable 'test_running' is defined locally in buzzer.c but is never updated by the actual test status. The test status is tracked separately in control.c. This means test_running will always be false after initialization (except during beep functions), which could cause the continuous_beep function to not work correctly. Consider removing this redundant state tracking and relying solely on control_get_test_status().

Copilot · 2025-12-20T17:34:37Z

app/other/solids_test/src/flash_storage.c

+static const struct device *flash_dev = DEVICE_DT_GET(DT_ALIAS(storage));
+static uint32_t current_test_number = 0;
+static off_t current_write_addr = 0;
+static char test_type[] = "terminal";


The variable 'test_type' is declared as a static array with a fixed size of 9 bytes (sizeof "terminal" + null terminator), but line 194 attempts to write "meep" into it using snprintf with sizeof(test_type). While "meep" fits within the array, the array size should be defined based on the maximum expected string length to make the code more maintainable. Consider defining a constant for the test type buffer size or using a larger fixed size.

Copilot · 2025-12-20T17:34:38Z

app/other/solids_test/include/adc_reading.h

+ * @brief Struct to hold one ADC sample
+ */
+struct adc_sample {
+    uint32_t timestamp; /** Timestamp the sample was recorded in ms */


The comment states the timestamp is in ms (milliseconds), but the actual implementation at line 127 uses k_ticks_to_us_near32() which converts to microseconds (μs), not milliseconds. This documentation inconsistency could lead to incorrect data interpretation.

Suggested change

uint32_t timestamp; /** Timestamp the sample was recorded in ms */

uint32_t timestamp; /** Timestamp the sample was recorded in µs (microseconds) */

Copilot · 2025-12-20T17:34:38Z

app/other/solids_test/src/control.c

+    for (int i = 0; i < num; i++) {
+        adc_read_one(&adc_val);
+        uint32_t t = k_uptime_ticks() - start;
+        shell_print(shell, "%u, %d", k_ticks_to_us_near32(t), adc_val);


Missing space after comma in the format string. Should be "%u, %d" for consistency with the output format used elsewhere in the codebase (e.g., line 247 in flash_storage.c).

Copilot · 2025-12-20T17:34:38Z

app/other/solids_test/include/button.h

+void button_pressed(const struct device *dev, struct gpio_callback *cb, uint32_t pins);
+
+/**
+ * @brief Interrupt function that toggles ematch and does something else


The documentation states this function "toggles ematch and does something else", which is vague and unhelpful. The actual function sets ematch high when the key switch is active and manages the buzzing state. The documentation should clearly describe what the function does, such as "Enables ematch when key switch is activated and triggers warning beep if no test is running".

Suggested change

* @brief Interrupt function that toggles ematch and does something else

* @brief Interrupt handler that enables ematch when the key switch is active and manages the buzzer warning state

lol I forgot to change this comment from the placeholder my bad

…e len)

…rent test type modification

…ers, thread defines in main

AarC10 · 2025-12-29T05:52:10Z

app/other/solids_test/src/flash_storage.c

+    int ret;
+    ret = flash_erase(flash_dev, FLASH_METADATA_ADDR, FLASH_METADATA_SIZE);


Just simplify to one line

AarC10 · 2025-12-29T05:52:29Z

app/other/solids_test/src/flash_storage.c

+        size_t i = 0;
+        size_t pages = 0;
+
+        while (1) {


while (true)

cowsed · 2025-12-29T13:20:11Z

app/other/solids_test/include/button.h

+int button_switch_init();
+
+/**
+ * @brief Interrupt handler that starts a test when TX pin is pulled high


i think button_pressed and key_switch_state both can be internal to button.c and need not be exposed in the header.

zizz-0 and others added 30 commits October 19, 2025 11:59

solids project setup

49c2fb6

solids test code builds, untested

4693d9f

new overlay and adc channel cfg

94a8979

folder renamed

e2b3276

broken :(

a178bd6

fixes n things

0904818

merge conflict :(

abea238

Merge branch 'zizz/solids-test-board' of https://github.com/RIT-Launc…

1c08ff9

…h-Initiative/FSW into zizz/solids-test-board

halfway working maybe

f04f221

renamed fr this time

6b274fe

flash dump works, adc reading not working fast enough :(

93401a2

got left bc of merge

1ec5a0c

erase command, writing in blocks

4ffc9bb

adc fine tuning/optimiaztion

56739fc

buzzer accidentally auto formatted (sorrry)

instrumenting

226d095

DMAing it up

170f331

ematches and reading misses

0a85a5d

stashing changes - button interrupt (not done)

b9bb43f

Merge branch 'zizz/solids-test-board' of https://github.com/RIT-Launc…

9e82edd

…h-Initiative/FSW into zizz/solids-test-board

solids test probably done. buzzer unfortunately on. tx/rx button work…

8618a1f

…ing. test read one implemented 🎷🐛

checkl on flash test

6736b06

set ematch/estop shell cmds, ematch set 500ms into adc reading instea…

a43ff19

…d of 250

Merge branch 'zizz/solids-test-board' of https://github.com/RIT-Launc…

feea5ce

…h-Initiative/FSW into zizz/solids-test-board

formatting

554c8f0

shell print n

d82bf52

formatting, comments, button functions moved

7d246a5

sys init

2bbf7ef

main commands print update

9837e91

function definitions and sys_init

a48a8bc

even more formatting

5dcc0ee

zizz-0 added 7 commits November 10, 2025 19:31

logging fixed

3955b55

don't blow up solids members code (calibration name, key switch, very…

d2b1cf4

… long beep)

buzzer task tested

2c33dde

comments, formatting, updated continuous_beep

827931f

print whether test is terminal or meep, terminal formatting

9ae27ca

comments, rx/tx gpio interrupt conf flags

afdbe5d

set buzzer back to buzzer instead of led :(

c65f31d

zizz-0 requested a review from a team as a code owner December 9, 2025 01:50

cowsed reviewed Dec 9, 2025

View reviewed changes

zizz-0 added 3 commits December 19, 2025 15:25

swap tx/rx and button/key_switch

be6d159

clarifying comments

f71d876

sample name

038a463

cowsed reviewed Dec 20, 2025

View reviewed changes

AarC10 requested changes Dec 20, 2025

View reviewed changes

AarC10 requested a review from Copilot December 20, 2025 17:30

Copilot started reviewing on behalf of AarC10 December 20, 2025 17:30 View session

Copilot AI reviewed Dec 20, 2025

View reviewed changes

zizz-0 added 3 commits December 28, 2025 20:23

clarified comments, fixed values (spi max freq, added const calib nam…

2ea3dde

…e len)

clarified more comments, fix print statements, protect against concur…

ca4a05d

…rent test type modification

void*, void*, void* on tasks to avoid warnings, tasks exposed in head…

40e4dd6

…ers, thread defines in main

AarC10 approved these changes Dec 29, 2025

View reviewed changes

cowsed reviewed Dec 29, 2025

View reviewed changes

+    ret = gpio_pin_configure_dt(&ematch, GPIO_OUTPUT_INACTIVE);
+    if (ret < 0) {
+        LOG_ERR("Failed to conf ematch pin :(");
+        return -1;
+    }

	shell_print(shell, "Pick a valid test [0-29]");
	shell_print(shell, "Pick a valid test [0-%u]", MAX_TESTS - 1);


		K_THREAD_DEFINE(storage_thread, 2048, flash_storage_thread_entry, NULL, NULL, NULL, STORAGE_THREAD_PRIORITY, 0, 1000);

		static char calibration[32];

	uint32_t timestamp; /** Timestamp the sample was recorded in ms */
	uint32_t timestamp; /** Timestamp the sample was recorded in µs (microseconds) */

	* @brief Interrupt function that toggles ematch and does something else
	* @brief Interrupt handler that enables ematch when the key switch is active and manages the buzzer warning state

		int ret;
		ret = flash_erase(flash_dev, FLASH_METADATA_ADDR, FLASH_METADATA_SIZE);

Solids Test #348

Are you sure you want to change the base?

Solids Test #348

Uh oh!

Conversation

zizz-0 commented Dec 9, 2025

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

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!