Skip to content

Create CAN Communication handler class for inverters#73

Draft
Hibyehello wants to merge 23 commits into
mainfrom
inverter-can-comm
Draft

Create CAN Communication handler class for inverters#73
Hibyehello wants to merge 23 commits into
mainfrom
inverter-can-comm

Conversation

@Hibyehello

@Hibyehello Hibyehello commented Jan 28, 2026

Copy link
Copy Markdown
Contributor

A basic framework that I cobbled together for inverter communication. I made a virtual class to be the base.

Adds

  • Start function
  • Stop function
  • HandleFrame function
  • Heartbeat for DTIX50 inverters

Putting this here just to get some reviews, and for any brainstorming

@Hibyehello Hibyehello requested a review from a team as a code owner January 28, 2026 04:46
@Hibyehello Hibyehello marked this pull request as draft January 28, 2026 04:46

@mcgaryp mcgaryp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome start!

I haven't thought about it more than just stating it here. Let's write unit tests of this code. Let's think of how we expect it to be called and write the tests around that :) looking very testable so far. We may need to add a few small tweaks to it as we explore testing

Great work

Comment thread lib/inverter/controller.h
Comment thread lib/inverter/DTIX50/can.cpp Outdated
Comment thread lib/inverter/DTIX50/can.cpp Outdated
Comment thread lib/inverter/can.h Outdated
Comment thread lib/inverter/DTIX50/can.cpp Outdated
// Set up the drive disable command
Command::SetDriveEnable command = { 0x0, 0xFFFFFFFFFFFFFF };

Frame frame(0x0C52, &command);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to get the dictionary of messages to IDs figured out at some point. Doesn't need to be now but let's put some todos in here as comments to utilize that dictionary or what ever we come up with to aid in creating forms that way we don't have to know the ids of messages when we creat them

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still see this as a relevant item to address we need to do it now or make sure we do not lose this requirement and do it later

Comment thread lib/inverter/DTIX50/controller.h
Comment thread lib/inverter/DTIX50/can.cpp Outdated
Comment thread lib/inverter/can.h Outdated
Comment thread lib/inverter/DTIX50/can.h Outdated
Comment thread lib/inverter/DTIX50/can.cpp Outdated
Comment thread lib/inverter/DTIX50/can.h Outdated
Comment thread lib/core/thread/i_thread_strategy.h

@mcgaryp mcgaryp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's comb through the new functions and decide which return values and parameters should be immutable in or out of the functions we are writing.

Comment thread test/mocks/strategies/native_thread_strategy.h Outdated
Comment thread lib/core/thread/i_thread_strategy.h Outdated
Comment on lines +10 to +11
virtual void setup(const char* name = nullptr, uint32_t priority = 0, uint32_t attributes = 0) = 0;
virtual uint32_t create(taskFunc task, void* argument) = 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the difference between setup and create functions?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setup I use since FreeRTOS and CMSIS both have an attributes that get passed to the thread/task and I had the setup function initialize that before so that the create command could be more concise.

Comment thread lib/core/thread/i_thread_strategy.h
Comment thread lib/inverter/DTIX50/controller.cpp
Comment thread lib/inverter/controller.h Outdated
Comment on lines +19 to +21
std::unique_ptr<Core::iLockStrategy> m_shouldStop_mut;
std::unique_ptr<Core::iThreadStrategy> m_thread;
Service *m_canService;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want to discuss the difference between unique pointer and *. Mostly to remind me but Also as some documentation is anyone wants to know in the future.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unique pointers mean that the pointer will be cleaned up by RAII so when the class that owns the unique pointers will automatically get cleaned up on that classes destruction. Unique pointers also get destroyed when the scope dies, since it's all RAII handled. The entire goal is to avoid memory leaks if we forget to free the pointer. Do note, someone else used unique pointers first and that made me remember them

Comment thread lib/inverter/controller.h
Comment thread lib/core/lock/cmsis_os2_lock_strategy.h
Comment thread lib/core/thread/cmsis_os2_thread_strategy.h
Comment thread lib/core/lock/cmsis_os2_lock_strategy.h Outdated
Comment thread lib/core/thread/cmsis_os2_thread_strategy.h Outdated
Comment thread test/test_inverter/DTIX50/test_inverter_communicate.cpp Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm interested in seeing a test for the heart beat.
Something I'd like to do this semester is create some kind of mock CAN bus. where we can mock the can behavior and test can functions... maybe we have enough if we create a mock can service? we just need to make the object. and test it out... future thoughts perhaps.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was trying to think of a way to test for it, I considered making a method to check that the thread was running, but we really do need a mock CAN bus which would be great.

@Hibyehello

Hibyehello commented Jan 30, 2026

Copy link
Copy Markdown
Contributor Author

Weird test fail, it worked so many times in a row on my mac. I think I might blame std::thread for the MacOS test being inconsistent

@Hibyehello

Hibyehello commented Jan 31, 2026

Copy link
Copy Markdown
Contributor Author

Update on tests, I accidentally introduced undefined behavior from not initializing m_shouldStop

}

// Sends a drive enable every ~250 milliseconds so the car doesn't stop
void Controller::heartbeat(void* s) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want the heartbeat function to be thread safe. If the function is called multiple times we should only have one "heartbeat" running at a time not multiple

}

// Just interprets message22 and sends the error codes
FaultCodes Controller::getFaultCode(const Frame &frame) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on the fence with the need for this function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants