Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FWT-7 I'll VCing U in court. #2

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

aclowmclaughlin
Copy link
Collaborator

Initial setup for the main method for both the Hardmon and MCUC main targets.
Implemented:
Powertrain (raw) CAN stuff
Simulink model integration
Main control loops for both models.
Reading my own code and having a seizure
Unimplemented:
Accessory (CANopen) CAN stuff
Correct Powertrain CAN message ids & format (all my HIB format stuff is a total guess)
Happiness

…en the two pinouts, will fix), and stubbed out the Hardmon process method.
…he input & output data for the MCUC, including the Powertrain CAN line (lots of assumptions), but NOT including the canOpen.
…ing information from powertrain CAN messages.
… is so much stuff in this that is unfinished/just makes assumptions about how stuff in the future will be implemented, mostly HIB and all the can IDs are just complete guesses). Also none of the accessoryCan stuff is implemented cause it's on CANopen and we have yet to determine all the dictionaries we need to use, and exactly how it's gonna work.
@aclowmclaughlin aclowmclaughlin requested review from a team and mjmagee991 April 27, 2024 15:55
@mjmagee991 mjmagee991 changed the title I'll VCing U in court. FT2-291 I'll VCing U in court. Jun 20, 2024
@mjmagee991 mjmagee991 changed the title FT2-291 I'll VCing U in court. FWT-7 I'll VCing U in court. Jul 5, 2024
Copy link
Contributor

@mjmagee991 mjmagee991 left a comment

Choose a reason for hiding this comment

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

Definitely have some ideas of things to change, but this is a great start

Copy link
Contributor

Choose a reason for hiding this comment

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

Move these to a folder called datasheets/. This isn't a perfect solution, but it's better than having a unique folder like this

/**
* Driver for the Hardware Monitor
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

struct reqGPIO {
//model input pins
IO::GPIO& ignitionCheckGPIO;
IO::GPIO& ignition3V3GPIO;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are there two ignition GPIOs? Is one of them for self test?

IO::GPIO& lvssStatus3V3GPIO;
IO::GPIO& mcStatusGPIO;

IO::GPIO& ucStateZeroGPIO;
Copy link
Contributor

Choose a reason for hiding this comment

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

An array of GPIOs would be better here


IO::GPIO& eStopCheckGPIO;
IO::GPIO& watchdogGPIO;
IO::GPIO& eStop3V3GPIO;
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, why duplicates?

// discuss it more with the EES and maybe Matt.
if (inverterDischarge) {
powertrainCAN.setMCInverterDischarge(true);
powertrainCAN.sendMCMessage();
Copy link
Contributor

Choose a reason for hiding this comment

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

The Hardmon should only send a message if we've determined the MCUC is no longer trustworthy. Is that decided by the model or by our code?

inverterEnable = outputs.Inverter_EN_CAN;
ucFault = outputs.Fault;
watchdog = outputs.Watchdog;
ucState[0] = outputs.uC_State & 0b00000001;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're going to use bit-shifting here, you don't need to specify 1 like with bits

Suggested change
ucState[0] = outputs.uC_State & 0b00000001;
ucState[0] = outputs.uC_State & 0x01;

//We will send accessory CAN SelfTest message over CANopen
//Send the powertrainCanSelfTest message
if (powertrainCanSelfTestOut) {
powertrainCAN.sendUCSelfTestMessage();
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking that self test should be handled entirely before we get to the process loop

gpios.canSelfTestGPIO.writePin(canSelfTest ? IO::GPIO::State::HIGH : IO::GPIO::State::LOW);

//Send the Motor Controller CAN message (set values first)
powertrainCAN.setMCInverterEnable(inverterEnable);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to pass in a struct to send the message instead of setting a bunch of variables independently?


VCU::MCUC mcuc(gpios, ptCAN);
ptCAN.addIRQHandler(powertrainCANInterrupt, reinterpret_cast<void*>(mcuc.getPowertrainQueue()));

Copy link
Contributor

Choose a reason for hiding this comment

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

Might handle self-test here

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