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

I2C Driver Implementation with RTOS STM32 HAL #32

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

Conversation

taisirhassan
Copy link

I2C Module Implementation
Implements RTOS-friendly I2C interface for processor board sensors. Key features:

Interrupt-based transfers (no polling)
Configurable timeouts per device
Simple peripheral-based interface (no need to manage HAL handles)
One device per bus design from latest board revision

@taisirhassan taisirhassan self-assigned this Nov 20, 2024
@Redstone-ray Redstone-ray self-requested a review November 20, 2024 02:03
@Redstone-ray
Copy link
Collaborator

I know this is still WIP, but IMU, BARO and MAG are not three different peripherals, please double check the relevant documentation to understand which three IMUs we are using

@taisirhassan
Copy link
Author

I know this is still WIP, but IMU, BARO and MAG are not three different peripherals, please double check the relevant documentation to understand which three IMUs we are using

Updated to use the right IMUs, had a general one that I forgot to change. Thanks for pointing it out.

@celery6 celery6 self-requested a review November 20, 2024 03:43
Copy link
Collaborator

@Redstone-ray Redstone-ray left a comment

Choose a reason for hiding this comment

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

Thanks for your update, quick question

  • you indicated a max len of "max 255 byte", but len is a uint16_t, which can exceed your limit of 255?
  • should the data be modifiable? if not, declare it as a constant

@taisirhassan
Copy link
Author

Thanks for your update, quick question

  • you indicated a max len of "max 255 byte", but len is a uint16_t, which can exceed your limit of 255?
  • should the data be modifiable? if not, declare it as a constant

You're right on both points. I changed len to uint8_t to match the 255-byte limit and made data const in write_reg() since it's read-only. I kept data non-const in read_reg() since it needs to be written to the buffer. Thanks.

@taisirhassan taisirhassan linked an issue Nov 20, 2024 that may be closed by this pull request
Copy link
Collaborator

@celery6 celery6 left a comment

Choose a reason for hiding this comment

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

few nits. also left a comment in design doc abt updating the diagram. otherwise this looks great

src/drivers/i2c/i2c.h Show resolved Hide resolved
src/drivers/i2c/i2c.h Show resolved Hide resolved
@taisirhassan taisirhassan requested a review from celery6 December 20, 2024 04:30
} i2c_device_t;

// Initialize an I2C device
// device: I2C device to initialize

Choose a reason for hiding this comment

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

If we're commenting in this level of detail might as well use doxygen

Copy link
Author

Choose a reason for hiding this comment

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

honestly had no idea on what level of detail I should've utilized but would doxygen be good atp if we're assuming the user isn't familiar with the design document? @celery6

Copy link
Collaborator

Choose a reason for hiding this comment

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

any code on its own should always contain documentation regardless of whether design doc exists or not.

Idc about doxygen or any other style tbh but sure lets make doxygen style our standard from now on

@Joe-Joe-Joe-Joe
Copy link

How exactly do you register the callbacks? Are you going to write a dedicated callback for each i2c device, then have the init function pass those function pointers behind the scenes? Would it be cleaner (since you callbacks are just one-liners to give the correct semaphore aiui) to implement the global callback function and a simple switch-case based on which handle called the callback?

@taisirhassan
Copy link
Author

How exactly do you register the callbacks? Are you going to write a dedicated callback for each i2c device, then have the init function pass those function pointers behind the scenes? Would it be cleaner (since you callbacks are just one-liners to give the correct semaphore aiui) to implement the global callback function and a simple switch-case based on which handle called the callback?

I was planning on using a single global callback for the i2c module, as you said it's just the same callback logic just on different semaphores it would be a lot less repetitive.

Copy link
Collaborator

@celery6 celery6 left a comment

Choose a reason for hiding this comment

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

could change comment style to doxygen otherwise LGTM!!!

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.

i2c module design
4 participants