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

Feature/pca #179

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

Feature/pca #179

wants to merge 10 commits into from

Conversation

rubennoro
Copy link

@rubennoro rubennoro commented Oct 12, 2024

UPDATED OCT 15
-Removed any notice of HAL driver
-Changed the names of pointers

Notes

Any other notes go here

Test Cases

-Test in Cerberus

To Do

Any remaining things that need to get done

-PR for Cerberus feature/pca

Checklist

It can be helpful to check the Checks and Files changed tabs.
Please reach out to your Project Lead if anything is unclear.
Please request reviewers and ping on slack only after you've gone through this whole checklist.

  • All commits are tagged with the ticket number
  • No merge conflicts
  • All checks passing
  • Remove any non-applicable sections of this template
  • Assign the PR to yourself
  • Request reviewers & ping on Slack
  • PR is linked to the ticket (fill in the closes line below)

Closes #158 (issue #158 )

HAL_StatusTypeDef pca9539_read_reg(pca9539_t *pca, uint8_t reg_type,
uint8_t *buf) {
/*
HAL_StatusTypeDef pca9539_read_reg(pca9539_t *pca, uint8_t reg_type, uint8_t *buf) {
Copy link
Contributor

Choose a reason for hiding this comment

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

need to remove HAL_Status_TypeDef to make this agnostic - those are just enums to ints so can replace with int

typedef int(*I2C_WriteFuncPtr)( uint16_t address, uint8_t reg_type,
uint8_t data );
typedef int(*I2C_ReadFuncPtr)( uint16_t address, uint8_t reg_type,
uint8_t data );
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick but can we change names:

I2c write function ptr --> Write_Ptr
read func ptr ----> Read_Ptr

then the local versions from "local..." to just "read" and "write"

Copy link
Contributor

@dyldonahue dyldonahue left a comment

Choose a reason for hiding this comment

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

good progress, a few changes and it should be good. also remove any long comments of old code or questions once done


// Two Function Pointers
WritePtr Write;
ReadPtr Read;
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry more nitpick but just so all drivers are the same:

Write --> write
Read --> read

uint8_t dev_addr) {
pca->i2c_handle = i2c_handle;
pca->dev_addr = dev_addr << 1u; /* shifted one to the left cuz STM says so */
void pca9539_init(pca9539_t *pca, WritePtr writeFunc,
Copy link
Contributor

Choose a reason for hiding this comment

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

i2c Handle is STM HAL, so that should be removed here

Copy link
Contributor

Choose a reason for hiding this comment

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

the i2c_handle member of the pca type can be removed too

if (status) {
return status;
}
//*buf = (data & (1 << pin)) > 0; What to do with this?
Copy link
Contributor

Choose a reason for hiding this comment

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

i dont know without looking much what this does, but changing exisint stuff outside of HAL related info is not needed, so you can keep this logic the same as it was initially

HAL_StatusTypeDef pca9539_write_reg(pca9539_t *pca, uint8_t reg_type,
uint8_t buf) {
//Errors in write_pin and read_pin functions if the commented code left in
//Variables within function are unused
Copy link
Contributor

Choose a reason for hiding this comment

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

the logic of this function has been changed, hence the errors. Take a look at the diff - the old code first read the state of the pin with a read call, then set data_new to the opposite state, and then wrote that back to the device. If you undo changes to that logic and keep it the same as it was u shoudlnt get any errors

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.

[Driver] - Make PCA95 driver platform agnostic
2 participants