TASK 289 by aolgeirson · Pull Request #6 · UCDFS/TEENSY

aolgeirson · 2025-11-24T21:00:06Z

basic setup
pre-near-total rewrite
ballmer peak
vibe changes
removed motorhal
fixed compile errors
I have many questions
removed units library
ready for review I think

* Initial plan * Update README with comprehensive VCU system documentation for Teensy 4.1 Co-authored-by: swhelan123 <149515365+swhelan123@users.noreply.github.com> * Fix university name from UC Davis to University College Dublin Co-authored-by: swhelan123 <149515365+swhelan123@users.noreply.github.com> * Update README.md --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: swhelan123 <149515365+swhelan123@users.noreply.github.com>

Copilot

Pull request overview

This PR represents a significant rewrite of the motor controller implementation, transitioning from a previous motorhal architecture to a new Motor class-based design. The changes include removing the units library and restructuring the CAN communication layer for the Bamocar D3 inverter.

Changes:

Introduced new Motor class with integrated CAN communication and state management
Removed all existing safety logic, APPS, brake, dashboard, and speed limiting code from main loop
Added comprehensive README documentation for the Teensy 4.1 VCU system

Reviewed changes

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

Show a summary per file

File	Description
src/motor.cpp	New motor controller implementation with CAN handling, RTD management, and torque control
include/motor.h	Motor class interface with CAN definitions, error codes, and hardware configuration
src/main.cpp	Drastically simplified main loop - all functionality removed except motor.update()
include/main.h	Global Motor instance declaration with uninitialized brake pointer
README.md	Added comprehensive project documentation and setup instructions
compile_commands.json	Updated build configuration for new file structure

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

Copilot · 2026-01-27T16:25:14Z

+    uint16_t rawStatusWord;
+
+    int16_t rpmFeedback;
+    float dcBusVoltage;
+    bool warning_present;
+
+    struct ProcessedStatusWord {
+      bool enableBit;
+      bool readyBit;
+      bool faultActive;
+    } statusWord;
+    struct Warnings {
+      bool parameter_conflict;
+      bool special_cpu_fault;
+      bool rfe_input_missing;
+      bool aux_voltage_min;
+      bool feedback_signal;
+      bool warn_5;
+      bool motor_temp_limit;
+      bool igbt_temp_limit;
+      bool vout_saturation_max;
+      bool warn_9;
+      bool speed_resolution_limit;
+      bool check_ecode_id;
+      bool tripzone_glitch;
+      bool adc_sequencer;
+      bool adc_measurement;
+      bool bleeder_resistor_load;
+    } warnings;
+
+    bool error_present;
+    struct Errors {
+      bool eprom_read;
+      bool hw_fault;
+      bool rfe_input_missing;
+      bool can_timeout;
+      bool feedback_signal;
+      bool mains_voltage_min;
+      bool motor_overheat;
+      bool endstage_overheat;
+      bool mains_voltage_max;
+      bool critical_ac_current;
+      bool race_away;
+      bool ecode_timeout;
+      bool watchdog_reset;
+      bool ac_current_offset;
+      bool internal_hw_voltage;
+      bool bleed_resistor_overload;
+    } errors;


The FullCANFrame struct defines many fields (warnings, errors, etc.) that are never populated in the readCAN() method. Only rawStatusWord, statusWord, rpmFeedback, and dcBusVoltage are set. The unused fields should either be populated with actual data or removed to avoid confusion.

Suggested change

uint16_t rawStatusWord;

int16_t rpmFeedback;

float dcBusVoltage;

bool warning_present;

struct ProcessedStatusWord {

bool enableBit;

bool readyBit;

bool faultActive;

} statusWord;

struct Warnings {

bool parameter_conflict;

bool special_cpu_fault;

bool rfe_input_missing;

bool aux_voltage_min;

bool feedback_signal;

bool warn_5;

bool motor_temp_limit;

bool igbt_temp_limit;

bool vout_saturation_max;

bool warn_9;

bool speed_resolution_limit;

bool check_ecode_id;

bool tripzone_glitch;

bool adc_sequencer;

bool adc_measurement;

bool bleeder_resistor_load;

} warnings;

bool error_present;

struct Errors {

bool eprom_read;

bool hw_fault;

bool rfe_input_missing;

bool can_timeout;

bool feedback_signal;

bool mains_voltage_min;

bool motor_overheat;

bool endstage_overheat;

bool mains_voltage_max;

bool critical_ac_current;

bool race_away;

bool ecode_timeout;

bool watchdog_reset;

bool ac_current_offset;

bool internal_hw_voltage;

bool bleed_resistor_overload;

} errors;

uint16_t rawStatusWord{0};

int16_t rpmFeedback{0};

float dcBusVoltage{0.0F};

bool warning_present{false};

struct ProcessedStatusWord {

bool enableBit{false};

bool readyBit{false};

bool faultActive{false};

} statusWord{};

struct Warnings {

bool parameter_conflict{false};

bool special_cpu_fault{false};

bool rfe_input_missing{false};

bool aux_voltage_min{false};

bool feedback_signal{false};

bool warn_5{false};

bool motor_temp_limit{false};

bool igbt_temp_limit{false};

bool vout_saturation_max{false};

bool warn_9{false};

bool speed_resolution_limit{false};

bool check_ecode_id{false};

bool tripzone_glitch{false};

bool adc_sequencer{false};

bool adc_measurement{false};

bool bleeder_resistor_load{false};

} warnings{};

bool error_present{false};

struct Errors {

bool eprom_read{false};

bool hw_fault{false};

bool rfe_input_missing{false};

bool can_timeout{false};

bool feedback_signal{false};

bool mains_voltage_min{false};

bool motor_overheat{false};

bool endstage_overheat{false};

bool mains_voltage_max{false};

bool critical_ac_current{false};

bool race_away{false};

bool ecode_timeout{false};

bool watchdog_reset{false};

bool ac_current_offset{false};

bool internal_hw_voltage{false};

bool bleed_resistor_overload{false};

} errors{};

Copilot · 2026-01-27T16:25:15Z

+    // Errors occupy the 0x8F low bitmask, whereas warning occupy the 0x8F high
+    // bitmask
+    //
+    // All avalible on page 148


The comment "All avalible on page 148" contains a spelling error. "avalible" should be corrected to "available".

Suggested change

// All avalible on page 148

// All available on page 148

Copilot · 2026-01-27T16:25:15Z

+std::optional<bool> Motor::getFromBitfield(uint16_t bitfieldAddr, uint8_t bit) {
+
+  CAN_message_t msg;
+  while (Can1.read(msg)) {
+    if (msg.id != BAMOCAR_TX_ID || msg.len < 3)
+      continue;
+    const uint8_t reg = msg.buf[0];
+    if (reg == bitfieldAddr) {
+      const uint16_t word = uint16_t(msg.buf[1]) | (uint16_t(msg.buf[2]) << 8);
+      // The bitshift is required to look at both a low and high message. Many
+      // bitfields are split into two groups, such as warnings and errors.
+      return std::optional<bool>{(word & bit) != 0};
+    }
+  }
+  return std::nullopt;
+}


The getFromBitfield method blocks while reading from the CAN bus (lines 90-100) without any timeout mechanism. The comment at line 162 in motor.h acknowledges this: "Currently risks blocking everything and starving the thread". This could cause the system to hang if no matching CAN message is received. Consider adding a timeout or making this non-blocking.

Copilot · 2026-01-27T16:25:15Z

+    return;
+  }
+
+  if ((!brake_active || !*brake_active) || !rtdButtonPressed()) {


The condition checks "(!brake_active || !*brake_active)" at line 145. If brake_active is nullptr (as it is in main.h), this will cause undefined behavior when attempting to dereference it. The code should validate that brake_active is not null before dereferencing, or use a safer pattern like std::optional.

Copilot · 2026-01-27T16:25:16Z

+  if (desired != lastTorque) {
+    set(ADDRS::SETPOINT, desired);
+    lastTorque = desired;


The setTorque method accepts a double parameter named "desired" but compares it with lastTorque (an int16_t) at line 248. This comparison may not work as expected due to type mismatch. The value is also passed to set() which takes an int. Consider using consistent types throughout or explicitly document the conversion behavior.

Suggested change

if (desired != lastTorque) {

set(ADDRS::SETPOINT, desired);

lastTorque = desired;

// Torque values are ultimately sent as integers; convert explicitly

const int16_t desiredTorque = static_cast<int16_t>(desired);

if (desiredTorque != lastTorque) {

set(ADDRS::SETPOINT, desiredTorque);

lastTorque = desiredTorque;

Copilot · 2026-01-27T16:25:18Z

+  // CHANGEME
+  static constexpr double MAX_TORQUE = 0.0;


The MAX_TORQUE constant is set to 0.0 with a CHANGEME comment. This will cause all torque requests above 0.0 to return MotorResponse::OVERTORQUE, effectively disabling motor control. This must be configured with the correct value before the code can be used.

Suggested change

// CHANGEME

static constexpr double MAX_TORQUE = 0.0;

// Maximum allowed motor torque in newton-meters.

// Configure this to match the motor/inverter torque limit before deployment.

static constexpr double MAX_TORQUE = 300.0;

Copilot · 2026-01-27T16:25:19Z

+ * @brief Manages Bamocar D3 inverter communications and torque control over
+ * CAN.
+ * @author Shane Whelan (UCD Formula Student)
+ * @date 2025/2026


The file header indicates "@Date 2025/2026" which appears to be a date range spanning two years. This is unusual for a file creation date. Consider using a more specific date format like "2025" or "January 2025".

Suggested change

* @date 2025/2026

* @date 2025

Copilot · 2026-01-27T16:25:19Z

+// This is a NULLPTR Placeholder and is not a safe permanant solution
+bool *brake_active;


The brake_active pointer is uninitialized and set to nullptr. This will cause undefined behavior when dereferenced in Motor::tryEnterRTD() at line 145 of motor.cpp. The pointer should be initialized to point to a valid boolean variable before being passed to the Motor constructor.

Suggested change

// This is a NULLPTR Placeholder and is not a safe permanant solution

bool *brake_active;

// Backing storage for the brake active flag; initialized to a known value.

bool brake_active_flag = false;

bool *brake_active = &brake_active_flag;

Copilot · 2026-01-27T16:25:19Z

+// Static member definitions
+/* FlexCAN_T4<CAN1, RX_SIZE_256, TX_SIZE_16> Motor::Can1;
+bool Motor::readyToDrive = false;
+bool Motor::faultActive = false;
+bool Motor::rtdRequestPending = false;
+uint32_t Motor::tPendingReady = 0;
+uint16_t Motor::statusWord = 0;
+int Motor::rpmFeedback = 0;
+float Motor::dcBusVoltage = 0.0f;
+int16_t Motor::lastTorque = 0;
+uint32_t Motor::tLastReissue = 0;
+uint32_t Motor::tBuzzerOff = 0;
+*/


The commented-out static member definitions at lines 5-16 should be removed if they are no longer needed. Leaving large blocks of commented code creates confusion and makes the codebase harder to maintain.

Suggested change

// Static member definitions

/* FlexCAN_T4<CAN1, RX_SIZE_256, TX_SIZE_16> Motor::Can1;

bool Motor::readyToDrive = false;

bool Motor::faultActive = false;

bool Motor::rtdRequestPending = false;

uint32_t Motor::tPendingReady = 0;

uint16_t Motor::statusWord = 0;

int Motor::rpmFeedback = 0;

float Motor::dcBusVoltage = 0.0f;

int16_t Motor::lastTorque = 0;

uint32_t Motor::tLastReissue = 0;

uint32_t Motor::tBuzzerOff = 0;

*/

Copilot · 2026-01-27T16:25:19Z

+/**
+ * @file motor_controller.cpp
+ * @brief Manages Bamocar D3 inverter communications and torque control over
+ * CAN.
+ * @author Shane Whelan (UCD Formula Student)
+ * @date 2025/2026
+ */


The file header comment at lines 17-23 describes this file as "motor_controller.cpp" but the actual filename is "motor.cpp". This inconsistency should be corrected.

Copilot AI and others added 10 commits October 21, 2025 13:58

basic setup

bbc82a9

pre-near-total rewrite

3de7927

ballmer peak

2fefab9

vibe changes

98b5051

removed motorhal

3a092cd

fixed compile errors

e960902

I have many questions

ed475b0

removed units library

8a61a76

ready for review I think

c3ce688

aolgeirson changed the base branch from main to testing November 24, 2025 21:01

cynosure and others added 4 commits November 24, 2025 22:27

fixed linker

0f4babb

added more errors and warnings

ccc06bd

partial removal of global variables

ff4ee38

Merge branch 'testing' into TASK-289

120c8a2

Copilot AI review requested due to automatic review settings January 27, 2026 16:18

Copilot started reviewing on behalf of swhelan123 January 27, 2026 16:18 View session

Copilot AI reviewed Jan 27, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TASK 289#6

TASK 289#6
aolgeirson wants to merge 14 commits intotestingfrom
TASK-289

aolgeirson commented Nov 24, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

-  if (desired != lastTorque) {
-    set(ADDRS::SETPOINT, desired);
-    lastTorque = desired;
+  // Torque values are ultimately sent as integers; convert explicitly
+  const int16_t desiredTorque = static_cast<int16_t>(desired);
+  if (desiredTorque != lastTorque) {
+    set(ADDRS::SETPOINT, desiredTorque);
+    lastTorque = desiredTorque;

-  // CHANGEME
-  static constexpr double MAX_TORQUE = 0.0;
+  // Maximum allowed motor torque in newton-meters.
+  // Configure this to match the motor/inverter torque limit before deployment.
+  static constexpr double MAX_TORQUE = 300.0;

		// This is a NULLPTR Placeholder and is not a safe permanant solution
		bool *brake_active;

-// This is a NULLPTR Placeholder and is not a safe permanant solution
-bool *brake_active;
+// Backing storage for the brake active flag; initialized to a known value.
+bool brake_active_flag = false;
+bool *brake_active = &brake_active_flag;

Conversation

aolgeirson commented Nov 24, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants