Skip to content

Conversation

@yyaatt0
Copy link
Contributor

@yyaatt0 yyaatt0 commented Jan 24, 2026

I removed the reset_mag() call inside the icm constructor because after a icm reset, the i2c master may not be ready. For a reliable and consistent startup of the icm device, reseting the mag outside of the constructor consistently works. Also included the reset_mag() call in the demo file as an example of how to use the driver.

Note: I have tried polling until the i2c master is ready, but still did not work.

Resolves #32

Copy link
Member

@kammce kammce left a comment

Choose a reason for hiding this comment

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

Thank you so much for the PR. from reviewing this, I think I see what's needed. You brought out the logic for resetting and initializing the driver on the outside so you could control how much time elapses before each step. What this tells me is that the constructor for the driver actually needs a steady_clock passed into it so it can delay the necessary amount of time. Otherwise, we put the burden on the developer using this driver:

  1. To remember to initialize these in the right order
  2. To delay the right amount between messages.

So what we should do is add that new constructor that takes a steady clock by reference and put a [[deprecated(message)]] attribute on the old constructor. That way people get a warning to not use it since it's dangerous.

Thank you again for taking this on.

@yyaatt0
Copy link
Contributor Author

yyaatt0 commented Jan 26, 2026

I've added the depreciated attribute to the old constructor as well as passing steady clock into the icm. I also needed to add helper functions to help allow time and ensure i2c is ready during the reset calls; specifically mag_whoami_ok() and reset_i2c_master().

If there are any critiques, let me know and I'll update accordingly.

Comment on lines 334 to 335
/* The Clock peripheral is used to provide delay until I2C master is ready. */
hal::steady_clock* m_steady_clock;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the steady clock after initialization? If not, then there is no need for us to hold the steady clock as a member.

@kammce
Copy link
Member

kammce commented Jan 26, 2026

Going to merge this now as we have things to fix on the CI side for sensor.

@kammce kammce merged commit 2946121 into libhal:main Jan 26, 2026
2 of 19 checks passed
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.

icm20948 reset_mag() function contains logic bug

2 participants