Develop by photonicslice · Pull Request #57 · UMSATS/tsat-utilities-kit

photonicslice · 2025-11-19T20:30:56Z

Fix critical race conditions as outlined in #55 and bug fixes and add comprehensive testing infrastructure for tx_cache

This commit addresses critical bugs in tx_cache and hardens can_wrapper with
queue validation

Phase 1: tx_cache Critical Bug Fixes

Bugs Fixed:

TxCache_Erase - Fixed incorrect circular buffer index translation
- Was comparing position to logical index (caused wrong items to be erased)
- Now correctly translates logical index to circular buffer position
- Location: Src/can_wrapper/tx_cache.c:57-75
TxCache_At - Fixed missing head offset in position calculation
- Was using index % TX_CACHE_SIZE without accounting for head
- Now correctly calculates position as (head + index) % TX_CACHE_SIZE
- Location: Src/can_wrapper/tx_cache.c:77-84

Defensive Programming:

Added TxCache_IsValid() helper with assertions for debug builds
Added null pointer checks on all public functions
Added bounds validation throughout
Added thread-safety documentation to source file

Testing Infrastructure:

Created comprehensive standalone test suite (15 tests, 200+ assertions)
Tests run without STM32 HAL or FreeRTOS dependencies
All tests passing with zero compiler warnings
Files created:
- tests/test_tx_cache.c - 700+ lines of comprehensive tests
- tests/tx_cache_standalone.c - Standalone implementation for testing
- tests/mock_can_message.h - Mock CAN types
- tests/mock_tx_cache.h - Mock cache header
- tests/Makefile - Build system with multiple targets
- tests/README.md - Complete testing documentation

Phase 2: can_wrapper Architecture Overhaul & Critical Bug Fixes

Architecture Transformation:

BEFORE (Original 3-thread design):

Error_Handler_Thread directly accessed s_tx_cache (race condition)
ISR directly accessed s_tx_cache via TxCache_Find/Erase (race condition)
Used osThreadFlagsSet for signaling
3 threads: Message Handler, Acknowledgement, Error Handler

AFTER (Single-owner pattern with command queue):

Cache_Manager_Thread is sole owner of s_tx_cache (race-condition-free)
ISR communicates via command queue (s_cache_command_queue)
All cache operations go through CacheCommand messages
4 threads: Message Handler, Acknowledgement, Cache Manager, Error Callback

Critical Bugs Fixed:

Bitwise OR instead of AND in ISR - Multiple locations

// BEFORE (always true!)
if (rx_behaviour | RX_ACK && !item.msg.is_ack)

// AFTER (correct bit checking)
if ((rx_behaviour & RX_ACK) && !item.msg.is_ack)

Impact: ACKs/messages were being sent incorrectly

Wrong index comparison in ISR

// BEFORE (misses index 0!)
if (index > 0) { TxCache_Erase(...); }

// AFTER
if (index >= 0) { TxCache_Erase(...); }

Impact: First cached message never removed on ACK

Incorrect osMessageQueuePut usage

// BEFORE (wrong - passing pointer to queue handle)
osMessageQueuePut(&s_ack_queue, &ack, 0U, 0U);

// AFTER (correct - queue handle is already a pointer)
osMessageQueuePut(s_ack_queue, &ack, 0U, 0U);

Missing parentheses in StdId calculation

// BEFORE (wrong operator precedence)
msg->priority << 5 & PRIORITY_MASK

// AFTER (correct)
(msg->priority << 5) & PRIORITY_MASK

Race Condition Elimination:

Implemented command queue pattern (CacheCommand enum)
Cache_Manager_Thread processes: ADD, REMOVE_BY_ACK, SHUTDOWN
ISR and other threads never directly touch s_tx_cache
Process_Expired_Items() isolated to cache manager thread

Queue Overflow Handling:

Added validation for all osMessageQueuePut() operations
Track overflows in diagnostic statistics structure
Prevents silent message/ACK loss
Locations: Transmit_Raw, ISR, Process_Expired_Items

RTOS Object Validation:

Changed RTOS_Init() return type from void to ErrorCode
Validate all queue and thread creation (9 objects total)
Fail fast on resource exhaustion instead of crashing later
Location: Src/can_wrapper/can_wrapper.c:137-216

Diagnostic Statistics System:

Added CANWrapper_Statistics struct with 8 counters:
- Queue overflow counts (4 queues)
- Operational metrics (TX/RX/ACK counts, timeouts)
Enables telemetry monitoring and queue tuning
Location: Src/can_wrapper/can_wrapper.c:69-80

Code Quality Improvements:

Use TxCache_At() for safe cache access (better encapsulation)
Removed unused Calculate_Next_Timeout() declaration
Added 40+ lines of thread-safety documentation
Comprehensive header documentation of threading model

New Error Codes:

ERR_CWM_RTOS_QUEUE_CREATE_FAILED
ERR_CWM_RTOS_THREAD_CREATE_FAILED
ERR_CWM_CACHE_QUEUE_FULL
ERR_CWM_ACK_QUEUE_FULL
ERR_CWM_MSG_QUEUE_FULL
ERR_CWM_ERROR_QUEUE_FULL

Testing:

tx_cache: 15 tests, 200+ assertions, all passed
Memory safety: Verified with Address Sanitizer (zero issues)
can_wrapper: Manual testing required (hardware-dependent)

…ture This commit addresses critical bugs in tx_cache and hardens can_wrapper with queue validation and diagnostic statistics for production-ready operation. ## Phase 1: tx_cache Critical Bug Fixes ### Bugs Fixed: 1. **TxCache_Erase** - Fixed incorrect circular buffer index translation - Was comparing position to logical index (caused wrong items to be erased) - Now correctly translates logical index to circular buffer position - Location: Src/can_wrapper/tx_cache.c:57-75 2. **TxCache_At** - Fixed missing head offset in position calculation - Was using `index % TX_CACHE_SIZE` without accounting for head - Now correctly calculates position as `(head + index) % TX_CACHE_SIZE` - Location: Src/can_wrapper/tx_cache.c:77-84 ### Defensive Programming: - Added TxCache_IsValid() helper with assertions for debug builds - Added null pointer checks on all public functions - Added bounds validation throughout - Added thread-safety documentation to source file ### Testing Infrastructure: - Created comprehensive standalone test suite (15 tests, 200+ assertions) - Tests run without STM32 HAL or FreeRTOS dependencies - All tests passing with zero compiler warnings - Files created: * tests/test_tx_cache.c - 700+ lines of comprehensive tests * tests/tx_cache_standalone.c - Standalone implementation for testing * tests/mock_can_message.h - Mock CAN types * tests/mock_tx_cache.h - Mock cache header * tests/Makefile - Build system with multiple targets * tests/README.md - Complete testing documentation ## Phase 2: can_wrapper Reliability Improvements ### Queue Overflow Handling: - Added validation for all osMessageQueuePut() operations - Track overflows in diagnostic statistics structure - Prevents silent message/ACK loss - Locations: Transmit_Raw, ISR, Process_Expired_Items ### RTOS Object Validation: - Changed RTOS_Init() return type from void to ErrorCode - Validate all queue and thread creation (9 objects total) - Fail fast on resource exhaustion instead of crashing later - Location: Src/can_wrapper/can_wrapper.c:137-216 ### Diagnostic Statistics System: - Added CANWrapper_Statistics struct with 8 counters: * Queue overflow counts (4 queues) * Operational metrics (TX/RX/ACK counts, timeouts) - Enables telemetry monitoring and queue tuning - Location: Src/can_wrapper/can_wrapper.c:69-80 ### New Error Codes: - ERR_CWM_RTOS_QUEUE_CREATE_FAILED - ERR_CWM_RTOS_THREAD_CREATE_FAILED - ERR_CWM_CACHE_QUEUE_FULL - ERR_CWM_ACK_QUEUE_FULL - ERR_CWM_MSG_QUEUE_FULL - ERR_CWM_ERROR_QUEUE_FULL ## Testing: - tx_cache: 15 tests, 200+ assertions, 100% pass rate - Memory safety: Verified with Address Sanitizer (zero issues) - can_wrapper: Manual testing required (hardware-dependent)

…frastructure" This reverts commit 5b0e47c.

…ture This commit addresses critical bugs in tx_cache and hardens can_wrapper with queue validation and diagnostic statistics for production-ready operation. ## Phase 1: tx_cache Critical Bug Fixes ### Bugs Fixed: 1. **TxCache_Erase** - Fixed incorrect circular buffer index translation - Was comparing position to logical index (caused wrong items to be erased) - Now correctly translates logical index to circular buffer position - Location: Src/can_wrapper/tx_cache.c:57-75 2. **TxCache_At** - Fixed missing head offset in position calculation - Was using `index % TX_CACHE_SIZE` without accounting for head - Now correctly calculates position as `(head + index) % TX_CACHE_SIZE` - Location: Src/can_wrapper/tx_cache.c:77-84 ### Defensive Programming: - Added TxCache_IsValid() helper with assertions for debug builds - Added null pointer checks on all public functions - Added bounds validation throughout - Added thread-safety documentation to source file ### Testing Infrastructure: - Created comprehensive standalone test suite (15 tests, 200+ assertions) - Tests run without STM32 HAL or FreeRTOS dependencies - All tests passing with zero compiler warnings - Files created: * tests/test_tx_cache.c - 700+ lines of comprehensive tests * tests/tx_cache_standalone.c - Standalone implementation for testing * tests/mock_can_message.h - Mock CAN types * tests/mock_tx_cache.h - Mock cache header * tests/Makefile - Build system with multiple targets * tests/README.md - Complete testing documentation ## Phase 2: can_wrapper Architecture Overhaul & Critical Bug Fixes ### Architecture Transformation: **BEFORE (Original 3-thread design):** - Error_Handler_Thread directly accessed s_tx_cache (race condition) - ISR directly accessed s_tx_cache via TxCache_Find/Erase (race condition) - Used osThreadFlagsSet for signaling - 3 threads: Message Handler, Acknowledgement, Error Handler **AFTER (Single-owner pattern with command queue):** - Cache_Manager_Thread is sole owner of s_tx_cache (race-condition-free) - ISR communicates via command queue (s_cache_command_queue) - All cache operations go through CacheCommand messages - 4 threads: Message Handler, Acknowledgement, Cache Manager, Error Callback ### Critical Bugs Fixed: 1. **Bitwise OR instead of AND in ISR** - Multiple locations ```c // BEFORE (always true!) if (rx_behaviour | RX_ACK && !item.msg.is_ack) // AFTER (correct bit checking) if ((rx_behaviour & RX_ACK) && !item.msg.is_ack) ``` Impact: ACKs/messages were being sent incorrectly 2. **Wrong index comparison in ISR** ```c // BEFORE (misses index 0!) if (index > 0) { TxCache_Erase(...); } // AFTER if (index >= 0) { TxCache_Erase(...); } ``` Impact: First cached message never removed on ACK 3. **Incorrect osMessageQueuePut usage** ```c // BEFORE (wrong - passing pointer to queue handle) osMessageQueuePut(&s_ack_queue, &ack, 0U, 0U); // AFTER (correct - queue handle is already a pointer) osMessageQueuePut(s_ack_queue, &ack, 0U, 0U); ``` 4. **Missing parentheses in StdId calculation** ```c // BEFORE (wrong operator precedence) msg->priority << 5 & PRIORITY_MASK // AFTER (correct) (msg->priority << 5) & PRIORITY_MASK ``` ### Race Condition Elimination: - Implemented command queue pattern (CacheCommand enum) - Cache_Manager_Thread processes: ADD, REMOVE_BY_ACK, SHUTDOWN - ISR and other threads never directly touch s_tx_cache - Process_Expired_Items() isolated to cache manager thread ### Queue Overflow Handling: - Added validation for all osMessageQueuePut() operations - Track overflows in diagnostic statistics structure - Prevents silent message/ACK loss - Locations: Transmit_Raw, ISR, Process_Expired_Items ### RTOS Object Validation: - Changed RTOS_Init() return type from void to ErrorCode - Validate all queue and thread creation (9 objects total) - Fail fast on resource exhaustion instead of crashing later - Location: Src/can_wrapper/can_wrapper.c:137-216 ### Diagnostic Statistics System: - Added CANWrapper_Statistics struct with 8 counters: * Queue overflow counts (4 queues) * Operational metrics (TX/RX/ACK counts, timeouts) - Enables telemetry monitoring and queue tuning - Location: Src/can_wrapper/can_wrapper.c:69-80 ### Code Quality Improvements: - Use TxCache_At() for safe cache access (better encapsulation) - Removed unused Calculate_Next_Timeout() declaration - Added 40+ lines of thread-safety documentation - Comprehensive header documentation of threading model ### New Error Codes: - ERR_CWM_RTOS_QUEUE_CREATE_FAILED - ERR_CWM_RTOS_THREAD_CREATE_FAILED - ERR_CWM_CACHE_QUEUE_FULL - ERR_CWM_ACK_QUEUE_FULL - ERR_CWM_MSG_QUEUE_FULL - ERR_CWM_ERROR_QUEUE_FULL ## Testing: - tx_cache: 15 tests, 200+ assertions, 100% pass rate - Memory safety: Verified with Address Sanitizer (zero issues) - can_wrapper: Manual testing required (hardware-dependent)

MeasureOneCodeTwice · 2026-01-15T02:15:46Z

@@ -0,0 +1,137 @@
+/** (c) 2024 UMSATS


I don't think having two copies of the implementation is a good idea.
To mock a file's imports we should get the linker to link a mock implementation.

MeasureOneCodeTwice · 2026-01-15T02:16:10Z

@@ -0,0 +1,35 @@
+/** (c) 2024 UMSATS


Could we move the mocks to a mocks folder

MeasureOneCodeTwice · 2026-01-15T02:18:23Z

+#define RX_CLEAR_TX_STORE 0b100
+#endif
+
+// Simple test framework macros


Logan and I had discussed using a testing framework. Not sure if we should use that.
Either way, we could move these assertions to a test utilities file

We'll discuss this in a workshop

MeasureOneCodeTwice · 2026-01-15T02:18:41Z

+
+// Integration into your main application
+// Add this to your main.c:
+/*


Commented out code

MeasureOneCodeTwice · 2026-01-15T02:25:17Z

+make clean && make asan
+```
+
+## Critical Bugs Fixed


Can get rid of bugs fixed. This PR will keep track of them.

MeasureOneCodeTwice · 2026-01-15T02:27:06Z

+This single-owner pattern ensures race-condition-free operation in the FreeRTOS environment.
+
+## License
+


@Koloss0 what's our license for this codebase?

MeasureOneCodeTwice · 2026-01-15T02:27:13Z

+2. Run test suite to verify changes
+3. Add new tests for new functionality
+
+## Compiler Warnings


Can get rid of this section and the next. As a developer you don't really need to know what warning flags are enabled.
I like the zero warnings tolerated though.

MeasureOneCodeTwice · 2026-01-15T02:27:56Z

@@ -0,0 +1,271 @@
+# TxCache Standalone Test Suite


Can get rid of overview and files section.

MeasureOneCodeTwice · 2026-01-15T02:29:17Z

+
+// Performance benchmark (basic)
+void test_basic_performance(void) {
+    printf("\n=== Basic Performance Test ===\n");


What is the point of this test?

It's just a smoke test to confirm the RTOS tick counter works and time is progressing normally. As noted in the comment, it's very rudimentary level testing. Real performance testing would require hardware timers for accurate microsecond-level measurements. The point was to confirm that osKernelGetTickCount() actually works and returns sensible values before relying on the timeouts and time-based operations elsewhere in CAN Wrapper(e.g. timeout handling in cache-manager) as I couldn't do the hardware testing.

Koloss0

I appreciate the work you put into this. However, there's just far too much in here for me to properly review. It's also addressing much more than the original issue and I don't feel comfortable merging it until I understand all the decisions that went into the unit testing stuff. We're going to have to split this into smaller pieces. Sorry about this. Let's discuss it in the next meeting.

Koloss0 · 2026-01-18T17:33:06Z

You have tsat-utilities-kit as a submodule of itself. This is definitely not right.

Koloss0 · 2026-01-18T17:39:19Z

@@ -0,0 +1,58 @@
+# Makefile for tx_cache standalone tests


Don't change it yet until we've discussed it. I was mainly suggesting we might be able to integrate our tests with the IDE better with CMake since it supports it as a build system. But I don't know for sure how well it'd work.

Koloss0 · 2026-01-18T17:39:53Z


-#include "tuk/debug/debug_logger.h"
-#include "tuk/debug/print.h"
+#include "../../Inc/tuk/debug/debug_logger.h"


Avoid backtracking in include paths.

Koloss0 · 2026-01-18T17:41:15Z

-#include <tuk/can_wrapper/tx_cache.h>
+#include <assert.h>
+#include <stddef.h>
+#include <../../Inc/tuk/can_wrapper/tx_cache.h>


Avoid backtracking in include paths.

Koloss0 · 2026-01-25T21:05:14Z

This main function shouldn't be here since TUK is only a library. You only need main for running the unit tests.

photonicslice added 12 commits October 23, 2025 17:37

Test with CI/CD Workflow

6dc6988

Fixed directory naming bugs

ff5dfc5

test workflow

c704494

fixed directories

c1e6729

fixed directories

bd9e190

directory namings

e8e64af

Updated test files

4763e4a

simplified test files

0600381

fixed directory problems

69df34b

Revert "Fix critical race conditions and add comprehensive testing in…

f003860

…frastructure" This reverts commit 5b0e47c.

photonicslice requested a review from Koloss0 November 19, 2025 20:33

MeasureOneCodeTwice requested changes Jan 15, 2026

View reviewed changes

Koloss0 requested changes Jan 18, 2026

View reviewed changes

Koloss0 closed this Jan 24, 2026

Koloss0 deleted the develop branch January 24, 2026 22:32

Koloss0 reviewed Jan 25, 2026

View reviewed changes

		This single-owner pattern ensures race-condition-free operation in the FreeRTOS environment.

		## License

Uh oh!

Conversation

photonicslice commented Nov 19, 2025

Phase 1: tx_cache Critical Bug Fixes

Bugs Fixed:

Defensive Programming:

Testing Infrastructure:

Phase 2: can_wrapper Architecture Overhaul & Critical Bug Fixes

Architecture Transformation:

Critical Bugs Fixed:

Race Condition Elimination:

Queue Overflow Handling:

RTOS Object Validation:

Diagnostic Statistics System:

Code Quality Improvements:

New Error Codes:

Testing:

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!

Choose a reason for hiding this comment

Uh oh!

Koloss0 left a comment • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Koloss0 left a comment •

edited

Loading