fix(AS5600): convert static variables to instance members #571
Conversation
…g sampling multiple sensors Replace static variables `prev_count` and `prev_time` with instance member variables to support multiple AS5600 sensor instances running simultaneously. Previously, all instances would share the same static variables, causing incorrect position and velocity calculations in multi-sensor configurations. - Add `prev_count_` and `prev_time_` as class member variables - Initialize `prev_count_` to 0 and `prev_time_` to current time - Remove static keyword from variables in `update()` method - Ensures each sensor instance maintains independent state
|
✅Static analysis result - no issues found! ✅ |
There was a problem hiding this comment.
Pull request overview
This PR converts static variables prev_count and prev_time to instance members to support multiple AS5600 sensor instances operating independently. Previously, all instances shared the same static variables, causing incorrect position and velocity calculations in multi-sensor configurations.
Key Changes
- Added
prev_count_andprev_time_as instance member variables initialized in the class definition - Updated
update()method to use instance members instead of static variables - Added documentation explaining the instance-specific nature of these variables
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Initialize prev_count_ to the first sensor reading in init() to avoid calculating an incorrect diff on the first update() call. Previously, prev_count_ defaulted to 0 while count_ was set to the actual sensor reading, causing a large erroneous jump in the accumulator on startup. - Set prev_count_ = count after reading initial angle - Ensures diff calculation is correct from the first update - Prevents accumulator corruption on initialization
rahgirrafi
changed the title
finger563
left a comment
finger563
left a comment
There was a problem hiding this comment.
Thanks for making some improvements to as5600! I've suggested some minor changes which should meet your needs and help the implementation better match the existing implementation / design of the mt6701 encoder component as well.
This way in the future if we find improvements to the design / implementation of either of these components, we can apply the improvements to both :)
- Replace std::chrono::high_resolution_clock with esp_timer_get_time() - Convert prev_time_ to uint64_t prev_time_us_ for microsecond precision - Store prev_count as local variable instead of class member
finger563
left a comment
finger563
left a comment
There was a problem hiding this comment.
This looks good! there's one code error where you're calling read instead of read_count, but other than that it looks good. Please when you make that fix also remove the additional whitespace that was pointed out by copilot.
As I mentioned below, once this PR is finished and merged, I'll create a fast-follow PR which makes some improvements to both as5600 and mt6701 for better consistency, interchangeability, and better defines a simple encoder interface.
…d and cleaned the code. - Used read_counts() method instead of the wrongly used non-existed read() method - Removed unnecessary trailing white spaces
|
I fixed the issue |
2 participants
Description
Replace static variables
prev_countandprev_timewith instance member variables.prev_count_andprev_time_as class member variablesprev_count_to 0 andprev_time_to current timeupdate()methodMotivation and Context
To support multiple AS5600 sensor instances running simultaneously. Previously, all instances would share the same static variables, causing incorrect position and velocity calculations in multi-sensor configurations.