Synching #580
Synching #580
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
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
- 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
…d and cleaned the code. - Used read_counts() method instead of the wrongly used non-existed read() method - Removed unnecessary trailing white spaces
Fix AS5600 count resolution and conversion - Correct COUNTS_PER_REVOLUTION to 4096 (12-bit) for AS5600 encoder - Remove incorrect bit shift in read_count(), now properly combines ANGLE_H and ANGLE_L for full 12-bit value - Update count-to-radian and count-to-degree conversions to match 4096 counts per revolution - Ensures correct angle readings and full 0–360° range from sensor
- Replace std::chrono with ESP32-native esp_timer_get_time() for velocity timing - Use uint64_t microsecond timestamps instead of chrono::time_point - Add esp_timer.h include for ESP-IDF timer functions - Maintain backward compatibility with ESP-IDF toolchain
|
✅Static analysis result - no issues found! ✅ |
There was a problem hiding this comment.
Pull request overview
This pull request corrects the AS5600 magnetic encoder implementation to properly handle the device's 12-bit resolution instead of incorrectly treating it as a 14-bit device. Additionally, it fixes concurrency issues by converting static variables to instance members to properly support multiple AS5600 sensors.
- Corrected COUNTS_PER_REVOLUTION from 16384 (14-bit) to 4096 (12-bit) to match the AS5600's actual resolution
- Fixed bit manipulation logic in read_count() to properly combine the 12-bit angle value from ANGLE_H and ANGLE_L registers
- Replaced static timing and count variables with instance members (prev_count_, prev_time_us_) to support multiple sensor instances
- Changed timing mechanism from std::chrono to esp_timer_get_time() for consistency
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int prev_count_{0}; | ||
| uint64_t prev_time_us_{0}; |
There was a problem hiding this comment.
The member variables prev_count_ and prev_time_us_ are accessed without mutex protection, but they're modified inside the update() method which is protected by base_mutex_. If update() can be called from multiple threads or if these variables are accessed from other methods, this could lead to race conditions. Consider using std::atomic for these variables or ensuring all accesses are protected by the mutex.
| int prev_count_{0}; | |
| uint64_t prev_time_us_{0}; | |
| std::atomic<int> prev_count_{0}; | |
| std::atomic<uint64_t> prev_time_us_{0}; |
|
|
||
| float raw_velocity = (float)(diff) / COUNTS_PER_REVOLUTION_F / seconds * SECONDS_PER_MINUTE; | ||
| velocity_rpm_ = velocity_filter_ ? velocity_filter_(raw_velocity) : raw_velocity; | ||
| static float max_velocity = 0.5f / update_period_.count() * SECONDS_PER_MINUTE; |
There was a problem hiding this comment.
The static variable max_velocity should also be converted to an instance member variable to properly support multiple AS5600 sensor instances, similar to how prev_count_ and prev_time_us_ were converted. Currently, all instances would share the same max_velocity value.
| static float max_velocity = 0.5f / update_period_.count() * SECONDS_PER_MINUTE; | |
| float max_velocity = 0.5f / update_period_.count() * SECONDS_PER_MINUTE; |
| int diff = count_ - prev_count_; | ||
| // update prev_count | ||
| prev_count = count_; | ||
| prev_count_ = count_; |
There was a problem hiding this comment.
Consider initializing prev_count_ in the init() method after reading the initial count value and before starting the task. This would ensure that the first velocity calculation in update() uses a valid baseline. Currently, prev_count_ is initialized to 0, which could cause an incorrect diff calculation on the first update if the encoder is not at position 0.
| uint64_t dt = now_us - prev_time_us_; | ||
| float seconds = dt / 1e6f; | ||
| prev_time_us_ = now_us; | ||
|
|
||
| if (seconds == 0.0f) { | ||
| seconds = update_period_.count(); | ||
| } |
There was a problem hiding this comment.
Consider initializing prev_time_us_ to the current time in the init() method before starting the task. This would prevent the first update() call from calculating an incorrect dt value based on the initial value of 0, which would result in a very large time delta.
| uint64_t dt = now_us - prev_time_us_; | |
| float seconds = dt / 1e6f; | |
| prev_time_us_ = now_us; | |
| if (seconds == 0.0f) { | |
| seconds = update_period_.count(); | |
| } | |
| float seconds; | |
| if (prev_time_us_ == 0) { | |
| // First update call: avoid using a huge dt from boot time, fall back to configured period. | |
| seconds = update_period_.count(); | |
| } else { | |
| uint64_t dt = now_us - prev_time_us_; | |
| seconds = dt / 1e6f; | |
| if (seconds == 0.0f) { | |
| seconds = update_period_.count(); | |
| } | |
| } | |
| prev_time_us_ = now_us; | |
| if (seconds == 0.0f) { | ||
| seconds = update_period_.count(); | ||
| } |
There was a problem hiding this comment.
The fallback to update_period_.count() when seconds == 0.0f may not provide accurate results on the first call to update(), as prev_time_us_ is initialized to 0. This means the first call will calculate an incorrect (very large) dt value. Consider initializing prev_time_us_ in the constructor or at the start of the first update call to avoid this issue.
1 participant
Description
Motivation and Context
How has this been tested?
Screenshots (if appropriate, e.g. schematic, board, console logs, lab pictures):
Types of changes
Checklist:
Software
.github/workflows/build.ymlfile to add my new test to the automated cloud build github action.Hardware