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

filled out channel definitions from altium definitions. (vc gpio) #143

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LucasZahlan
Copy link

No description provided.

@LucasZahlan LucasZahlan requested a review from JoshLafleur March 6, 2025 17:52
Copy link
Collaborator

@JoshLafleur JoshLafleur left a comment

Choose a reason for hiding this comment

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

None of the things you added will get configured because theyre all after HW_GPIO_COUNT. For more explanation look at the init function in components/shared/code/HW/HW_gpio.c. You also redeclared all of the pins that were already there. Id suggest you keep your definitions (with the fixes for my comments) but put them all before count and then remove what was already there. Also should update the excel aheet names while were at it.

Also what are your thoughts on specificying TACH for frequency inputs like the wheel speed and flow sensor?

HW_GPIO_DIG_SPARE_2,
HW_GPIO_DIG_SPARE3,
HW_GPIO_DIG_SPARE4,
HW_GPIO_LED,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Redeclared from above

@@ -17,4 +17,55 @@ typedef enum
HW_GPIO_CAN2_TX,
HW_GPIO_LED,
HW_GPIO_COUNT,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Count has to be the last index

Comment on lines +29 to +35
HW_GPIO_APPS_P1,
HW_GPIO_APPS_P2,
HW_GPIO_BR_POT,
HW_GPIO_L_SHK_DISP,
HW_GPIO_L_BR_TMP,
HW_GPIO_R_SHK_DISP,
HW_GPIO_R_BR_TMP,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adc inputs should be clearly labelled

HW_GPIO_R_BR_TMP,
HW_GPIO_AIN_PU1,
HW_GPIO_AIN_PU2,
HW_GPIO_BR_PR,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adc inputs should be clearly labeled

Comment on lines +44 to +45
HW_GPIO_RXD2,
HW_GPIO_TXD2,
Copy link
Collaborator

Choose a reason for hiding this comment

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

CAN should be specified

HW_GPIO_FLOW,
HW_GPIO_R_WS,
HW_GPIO_L_WS,
HW_GPIO_RUN,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Specify run button

Comment on lines 17 to 18
HW_GPIO_CAN2_TX,
HW_GPIO_LED,
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are previous defines that you are redefining. They need to be merged

HW_GPIO_AIN_PU1,
HW_GPIO_AIN_PU2,
HW_GPIO_BR_PR,
HW_GPIO_TEMP_1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

TEMP_1 is not descriptive. Is this board temperature?

Comment on lines +46 to +47
HW_GPIO_SEL2,
HW_GPIO_SEL1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

These select lines are for what?

Comment on lines +62 to +66
HW_GPIO_CS_IMU,
HW_GPIO_CS_SD,
HW_GPIO_SPI_SCK,
HW_GPIO_SPI_MISO,
HW_GPIO_SPI_MOSI,
Copy link
Collaborator

Choose a reason for hiding this comment

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

CS lines should be prefixed by SPI

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