I2c integration by jose5556 · Pull Request #15 · SEAME-pt/STM32_Microcontroller

jose5556 · 2026-02-04T11:16:58Z

Pull Request

Summary

This PR fixes several bugs related to I2C communication instabilities.
An emergency brake thread with maximum priority was added.
The definition of 1000 ticks per second was updated, and the logic for counting speed sensor values has also been improved.

Related issues

Related: #108

Type of change

Testing instructions

To test this implementation, a Raspberry Pi is required to send and receive CAN frames.
Instructions for setting up bidirectional communication are provided in the READMEs for both the Raspberry Pi and STM32 code.

- Added I2C3 configuration with timing parameters. - Removed FDCAN1 settings including baud rate and time segments. - Updated MCU IP list to include I2C3 and adjusted IP count. - Set PA7 and PC1 modes to I2C for proper signal configuration. - Updated function list to include I2C3 initialization.

… and i2c_pca9685.c

…s, and implement I2C bus scanning

…ad, update PCA9685 functions, and enhance I2C bus scanning

…rotocol definitions, and enhance motor control functionality

…ility functions for UART communication

…ge handling for servo angles

…rotocol.h and update servo thread for consistent servo angle testing

…ction to include brake parameter, add stopMotors function, and improve thread initialization logging

…eep for better thread management

… type in RX message to uint32_t, adjust data length handling, and modify sleep duration in speed sensor thread

…creation logs for better visibility

…meters for improved message handling

…proved performance

… for consistency

…line motor speed setting logic

…ts.md, requirements.md, and traceability_matrix.md

…fety

… speed and battery

…n thread

…ations

…xtension instructions

Copilot

Pull request overview

This PR integrates I2C communication for motor control (DC motors, servo) and battery monitoring, while making significant changes to system configuration including reducing the clock speed from 160MHz to 72MHz and increasing timer resolution from 100 to 1000 ticks per second.

Changes:

Adds I2C3 peripheral support with PCA9685 (PWM controller) and INA219 (voltage/current sensor) drivers
Implements new threads for DC motors, servo control, emergency brake, and battery monitoring
Updates system clock configuration from 160MHz to 72MHz and timer ticks from 100 to 1000 per second
Restructures code organization by separating CAN protocol definitions and moving thread implementations
Updates RPM calculation parameters (PPR changed from 20 to 40) and removes safety-critical documentation
Modifies FDCAN configuration by removing message filters and changes timer trigger from falling to rising edge

Reviewed changes

Copilot reviewed 33 out of 34 changed files in this pull request and generated 28 comments.

Show a summary per file

File	Description
tests/unit/test/test_ut_rpm.c	Updates test expectations for new PPR value (20→40), causing all RPM calculations to halve
docs/safety_critical/*	Removes all safety-critical documentation including requirements, traceability matrix, and test coverage
ThreadX_Os/ThreadX_Os.ioc	Reconfigures hardware: reduces system clock to 72MHz, switches from I2C2 to I2C3, modifies FDCAN timing
ThreadX_Os/Core/Src/i2c_*.c	Adds new I2C drivers for PCA9685 PWM controller and INA219 current sensor
ThreadX_Os/Core/Src/threads/motors/*	Implements motor control threads for DC motors, servo, and emergency brake
ThreadX_Os/Core/Src/threads/can/*	Refactors CAN RX/TX threads with new queue routing for motor commands
ThreadX_Os/Core/Src/main.c	Major hardware reconfig: clock from 160→72MHz, I2C3 init, FDCAN filters removed, TIM1 trigger edge changed
ThreadX_Os/Core/Src/app_threadx.c	Adds multiple queues for motor control and restructures initialization
ThreadX_Os/Core/Inc/*.h	Adds new headers for I2C devices and CAN protocol, updates thread priorities and counts

Comments suppressed due to low confidence (4)

ThreadX_Os/Core/Src/threads/can/tx_can.c:47

The CAN TX thread sleep interval has been reduced from 100 ticks to 1 tick. With the new 1000 ticks/second configuration, this means the thread wakes up every 1ms instead of every 1000ms (old config: 100 ticks / 100 ticks/sec = 1s). This is a 1000x increase in polling frequency. While this may provide more responsive CAN transmission, it will significantly increase CPU usage. Verify this is necessary and that the system can handle this overhead without impacting other threads.
ThreadX_Os/Core/Src/threads/speed_sensor.c:26
The line "ULONG cr1_reg = htim1.Instance->CR1;" has been commented out. This variable was used for debugging RPM values. However, the rpm_debug_print function at line 18 still expects this parameter in its prototype in app_threadx.h. If cr1_reg is no longer needed, consider also updating the rpm_debug_print function signature to remove this unused parameter, or re-enable reading cr1_reg if debugging is still needed.
docs/safety_critical/requirements.md:1
The safety-critical documentation including requirements.md, traceability_matrix.md, and index_requirements.md has been completely removed. This is a significant concern for a system that appears to handle emergency braking and motor control. If this is intentional, the PR description should explain why this documentation is no longer needed or where it has been moved. These documents contained traceability information, requirements, and test coverage metrics that are typically essential for safety-critical systems.
ThreadX_Os/Core/Src/threads/speed_sensor.c:45
The thread sleep time has been reduced from 400 ticks to 50 ticks. With TX_TIMER_TICKS_PER_SECOND now at 1000, this means the speed sensor thread wakes up every 50ms instead of every 40ms (the old 400 ticks with 100 ticks/second would have been 4000ms, but now 50 ticks with 1000 ticks/second is 50ms). This is a significant increase in sampling frequency. Verify this doesn't cause excessive CPU usage or interfere with other threads, especially given that this thread now has the lowest priority (NONE_PRIO).

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

Copilot · 2026-02-04T11:58:11Z

ThreadX_Os/Core/Src/main.c

  sSlaveConfig.SlaveMode = TIM_SLAVEMODE_EXTERNAL1;
  sSlaveConfig.InputTrigger = TIM_TS_TI1FP1;
-  sSlaveConfig.TriggerPolarity = TIM_TRIGGERPOLARITY_FALLING;
+  sSlaveConfig.TriggerPolarity = TIM_TRIGGERPOLARITY_RISING;


The trigger polarity for TIM1 has changed from TIM_TRIGGERPOLARITY_FALLING to TIM_TRIGGERPOLARITY_RISING. This fundamentally changes when speed sensor pulses are counted - now on rising edges instead of falling edges. This is a significant change that affects RPM measurement accuracy. Ensure this change is intentional and that the sensor hardware and mounting are compatible with rising edge triggering. This should be documented in the PR description as it affects calibration and may require hardware validation.

Copilot · 2026-02-04T11:58:11Z

ThreadX_Os/Core/Src/threads/can/rx_can.c

+        if (HAL_FDCAN_GetRxMessage(&hfdcan1, FDCAN_RX_FIFO0, &rxHeader, rx_data) == HAL_OK)
+        {
+            msg->type = rxHeader.Identifier;
+            msg->len = (rxHeader.DataLength < 8) ? rxHeader.DataLength : 8;


The condition at line 14 uses DataLength < 8, but previously it was <=. This changes behavior: now if DataLength equals 8, it will use 8 instead of clamping to 8. However, since 8 is already the maximum for CAN, this change is actually correct. But the use of < instead of <= might be confusing - consider using (rxHeader.DataLength <= 8) ? rxHeader.DataLength : 8 for clarity, or adding a comment explaining that DataLength is already validated to be <= 8 by the hardware.

Copilot · 2026-02-04T11:58:11Z

ThreadX_Os/Core/Src/threads/can/rx_can.c

+        {
+            msg->type = rxHeader.Identifier;
+            msg->len = (rxHeader.DataLength < 8) ? rxHeader.DataLength : 8;
+            memcpy(&msg->data, rx_data, msg->len);


At line 15, using memcpy with &msg->data as the destination is redundant - msg->data is already an array, so the & is unnecessary. It should be memcpy(msg->data, rx_data, msg->len). While this may work due to array decay, it's not idiomatic and could be confusing.

Suggested change

memcpy(&msg->data, rx_data, msg->len);

memcpy(msg->data, rx_data, msg->len);

Copilot · 2026-02-04T11:58:12Z

ThreadX_Os/Core/Src/init/init_can.c

 #include "app_threadx.h"

-static void TxHeartBeatConf(FDCAN_TxHeaderTypeDef *TxHeader);
 static void TxSpeedConf(FDCAN_TxHeaderTypeDef *TxHeader);
 static void TxBatteryConf(FDCAN_TxHeaderTypeDef *TxHeader);

 // Configuration of CAN frame for speed data
 void initCanFrames(t_canFrames *canFrames) 
 {
-    TxHeartBeatConf(&canFrames->tx_header_heart_beat);
    TxSpeedConf(&canFrames->tx_header_speed);
    TxBatteryConf(&canFrames->tx_header_battery);


The TxHeartBeatConf function and associated heartbeat CAN frame handling have been removed. The PR description mentions adding new features but doesn't explain why the heartbeat functionality was removed. Heartbeat messages are typically important for monitoring system health in real-time systems. If heartbeat functionality is no longer needed, this should be documented in the PR description. Otherwise, consider whether removing this safety monitoring feature is appropriate.

Copilot · 2026-02-04T11:58:12Z

ThreadX_Os/Core/Inc/app_threadx.h

+6 -> battery thread
+7 -> emergency brake thread
+*/
+#define THREAD_COUNT    7


The THREAD_COUNT is defined as 7, but only 6 threads are actually created (Speed sensor, CAN TX, CAN RX, DC Motors, Servo, Emergency Brake). The Battery thread (which would be the 7th) is commented out. This mismatch could lead to wasted memory allocation for the unused thread stack. Either enable the battery thread, or reduce THREAD_COUNT to 6.

Suggested change

6 -> battery thread

7 -> emergency brake thread

*/

#define THREAD_COUNT 7

6 -> emergency brake thread

*/

#define THREAD_COUNT 6

Copilot · 2026-02-04T11:58:16Z

ThreadX_Os/Core/Src/threads/motors/dc_motors.c

+                if (motor_set(MOTOR_LEFT, -throttle, 0) != HAL_OK)
+                    uart_send("DC Motors Thread: Failed to set LEFT motor speed\r\n");
+
+                if (motor_set(MOTOR_RIGHT, throttle, 0) != HAL_OK)


At line 29, the left motor throttle is negated (-throttle) while the right motor throttle at line 32 is not negated. This suggests differential drive where the left and right motors need opposite polarities. However, this is not documented in the code. Add a comment explaining why the left motor is negated, as this is a critical detail for understanding the motor control logic.

Copilot · 2026-02-04T11:58:16Z

ThreadX_Os/Core/Src/app_threadx.c

+  // Create Emergency Brake queue
+  UCHAR *can_emergency_brake_queue_memory = (UCHAR *)memory_ptr + QUEUE_SIZE * sizeof(t_tx_can_msg);
+  ret = tx_queue_create(&can_emergency_brake_queue, "CAN RX Queue", 
+                        sizeof(t_rx_can_msg) / sizeof(ULONG),
+                        can_emergency_brake_queue_memory, QUEUE_SIZE * sizeof(t_rx_can_msg));
+  if (ret != TX_SUCCESS)
+    uart_send("ERROR! Failed Emergency Brake queue creation.\r\n");
+
+  // Create I2C DC Motors queue
+  UCHAR *i2c_motors_queue_memory = can_emergency_brake_queue_memory + QUEUE_SIZE * sizeof(t_rx_can_msg);
+  ret = tx_queue_create(&i2c_dc_motors_queue, "I2C DC Motors Queue", 
                        sizeof(t_rx_can_msg) / sizeof(ULONG),
-                        rx_queue_memory, QUEUE_SIZE * sizeof(t_rx_can_msg));
+                        i2c_motors_queue_memory, QUEUE_SIZE * sizeof(t_rx_can_msg));
  if (ret != TX_SUCCESS)
-    uart_send("ERROR! Failed RX queue creation.\r\n");
+    uart_send("ERROR! Failed I2C DC Motors queue creation.\r\n");
+
+  // Create I2C Servo queue
+  UCHAR *i2c_servo_queue_memory = i2c_motors_queue_memory + QUEUE_SIZE * sizeof(t_rx_can_msg);
+  ret = tx_queue_create(&i2c_servo_queue, "I2C Servo Queue", 
+                        sizeof(t_rx_can_msg) / sizeof(ULONG),
+                        i2c_servo_queue_memory, QUEUE_SIZE * sizeof(t_rx_can_msg));
+  if (ret != TX_SUCCESS)
+    uart_send("ERROR! Failed I2C Servo queue creation.\r\n");


The queue creation error handling at lines 84-104 only sends UART messages but continues execution. If queue creation fails, the system will have undefined behavior when threads try to use these queues. After any queue creation failure, the function should return an error code immediately instead of continuing. Consider adding return ret; after each error message, or accumulating errors and checking at the end before initializing threads.

Copilot · 2026-02-04T11:58:16Z

ThreadX_Os/Core/Src/threads/can/rx_can.c

@@ -0,0 +1,53 @@
+#include "can_protocol.h"
+
+// CAN RX callback function


The function rx_receive is declared as static but the comment at line 3 says "CAN RX callback function". This is not actually a callback - it's a helper function called by thread_rx_can. The comment is misleading. Consider updating the comment to accurately describe it as a helper function that polls for CAN messages.

Suggested change

// CAN RX callback function

// Helper function that polls for and receives a CAN message

Copilot · 2026-02-04T11:58:17Z

ThreadX_Os/Core/Inc/i2c_pca9685.h

+static const t_motor_channel MOTOR_LEFT  = { 0, 1, 2 };
+static const t_motor_channel MOTOR_RIGHT = { 7, 5, 6 };


Static const variables MOTOR_LEFT and MOTOR_RIGHT are defined in a header file. While the const qualifier prevents modification, defining non-inline static variables in a header means each translation unit that includes this header will get its own copy of these variables. This wastes memory. Consider either making these extern const and defining them in the .c file, or using #define macros, or making them inline static const (C99+).

Suggested change

static const t_motor_channel MOTOR_LEFT = { 0, 1, 2 };

static const t_motor_channel MOTOR_RIGHT = { 7, 5, 6 };

#define MOTOR_LEFT ((t_motor_channel){ 0u, 1u, 2u })

#define MOTOR_RIGHT ((t_motor_channel){ 7u, 5u, 6u })

Copilot · 2026-02-04T11:58:17Z

ThreadX_Os/Core/Inc/app_threadx.h

 typedef struct s_threads {
  TX_THREAD thread;
-  uint8_t   stack[1024];
+  UINT      stack[1024];


The thread stack is changed from uint8_t to UINT type. UINT is typically defined as uint32_t, which means each stack element is now 4 bytes instead of 1 byte. With 1024 elements, this changes the stack size from 1KB to 4KB per thread. With 7 threads (THREAD_COUNT), this is 28KB total instead of 7KB. Verify this significant increase in memory usage is intentional and that the MCU has sufficient RAM. If 1KB stacks are sufficient, change back to uint8_t stack[1024].

AfonsoMota-132

the code additions and the features seems all good and tested, worked as expected, just be careful/review the small error of init that sometimes doesnt work

jose5556 added 30 commits January 22, 2026 10:59

Update I2C configuration to use I2C3 instead of I2C1 in app_threadx.h…

50eb69c

… and i2c_pca9685.c

Refactor I2C configuration: add I2C3 support, update PCA9685 function…

97f778e

…s, and implement I2C bus scanning

Implement I2C3 support and refactor motor control: add DC motors thre…

eeb60b6

…ad, update PCA9685 functions, and enhance I2C bus scanning

tab

a82b343

Refactor I2C and CAN integration: update I2C configuration, add CAN p…

f97c0c7

…rotocol definitions, and enhance motor control functionality

update I2C queue handling, enhance PCA9685 initialization, and add ut…

b88bfb8

…ility functions for UART communication

Refactor DC motors and servo threads: move motor test code, add messa…

bf1404a

…ge handling for servo angles

Refactor CAN and I2C message handling: move dlc_to_len array to can_p…

e83c4d0

…rotocol.h and update servo thread for consistent servo angle testing

Enhance motor control and thread initialization: update motor_set fun…

d222f24

…ction to include brake parameter, add stopMotors function, and improve thread initialization logging

Refactor motor and CAN threads: remove debug UART messages and add sl…

1a10116

…eep for better thread management

Update CAN message structures and enhance thread sleep timing: change…

698174a

… type in RX message to uint32_t, adjust data length handling, and modify sleep duration in speed sensor thread

Refactor thread initialization: uncomment DC Motors and Servo thread …

3740e70

…creation logs for better visibility

Update CAN configuration: enable AutoRetransmission and adjust IPPara…

71c7acf

…meters for improved message handling

Add CAN RX and TX thread implementations for message handling

5dae9c6

Add battery monitoring thread and update system configurations for im…

0e6c816

…proved performance

Refactor thread priority definitions and update thread creation calls…

0c6ed14

… for consistency

Refactor DC Motors thread: remove redundant error handling and stream…

891e567

…line motor speed setting logic

Add emergency break thread implementation and update thread count

ec58759

Remove outdated safety critical documentation files: index_requiremen…

8b624b7

…ts.md, requirements.md, and traceability_matrix.md

Add emergency break thread and modify CAN RX handling for improved sa…

9873009

…fety

Fix naming inconsistencies for emergency brake thread and related queues

3c7abee

Add emergency brake thread implementation for motor control

0e3f205

Remove heartbeat message type and update CAN frame configurations for…

34d9a18

… speed and battery

Refactor CAN message handling and improve error reporting in threads

061d33a

Refactor CAN message transmission and improve error handling in tx_ca…

af94b0a

…n thread

Update TIM1 trigger polarity from falling to rising

44933e5

Update Pulses Per Revolution definition to 40 for accurate RPM calcul…

4e768c8

…ations

Update timer tick reference to 1000 for improved timing accuracy

865c483

Remove TIM1 trigger filter and polarity configuration

21e10d3

jose5556 added 2 commits February 4, 2026 11:10

Remove I2C communication thread integration suggestion from project e…

32d62c8

…xtension instructions

Fix RPM calculation comments and expected values in unit tests

6316619

jose5556 requested a review from AfonsoMota-132 February 4, 2026 11:24

AfonsoMota-132 requested a review from Copilot February 4, 2026 11:45

Copilot started reviewing on behalf of AfonsoMota-132 February 4, 2026 11:46 View session

Copilot AI reviewed Feb 4, 2026

View reviewed changes

AfonsoMota-132 approved these changes Feb 4, 2026

View reviewed changes

jose5556 merged commit 9e545ed into main Feb 4, 2026
7 checks passed

jose5556 deleted the fet/i2c branch February 5, 2026 18:20

	memcpy(&msg->data, rx_data, msg->len);
	memcpy(msg->data, rx_data, msg->len);

		@@ -0,0 +1,53 @@
		#include "can_protocol.h"

		// CAN RX callback function

	// CAN RX callback function
	// Helper function that polls for and receives a CAN message

		static const t_motor_channel MOTOR_LEFT = { 0, 1, 2 };
		static const t_motor_channel MOTOR_RIGHT = { 7, 5, 6 };

Comments

Conversation

jose5556 commented Feb 4, 2026

Pull Request

Summary

Related issues

Type of change

Testing instructions

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

AfonsoMota-132 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AfonsoMota-132 left a comment •

edited

Loading