added: KOBA_EGPU mod for controlling cooling and lighting in the self…#5349
Open
ArthurKoba wants to merge 1 commit intowled:mainfrom
Open
added: KOBA_EGPU mod for controlling cooling and lighting in the self…#5349ArthurKoba wants to merge 1 commit intowled:mainfrom
ArthurKoba wants to merge 1 commit intowled:mainfrom
Conversation
…-mounted eGPU dock.
Contributor
WalkthroughIntroduces a new ESP32C3-based KOBA eGPU usermod providing temperature sensing from thermistors, dynamic fan PWM control based on configurable temperature thresholds, tachometer-based RPM measurement via ISR, and JSON configuration integration with WLED. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@usermods/KOBA_EGPU/koba_egpu.cpp`:
- Line 55: _pulses_count is subject to a race between the ISR and
_update_tachometer_speed; change its type to std::atomic<uint32_t> (include
<atomic>) and replace non-atomic reads/resets with atomic operations (e.g. use
fetch_exchange(0) or exchange(0) in _update_tachometer_speed to atomically
capture-and-clear the count) while keeping the ISR increment using fetch_add(1)
(or ++ on the atomic). Update all references to _pulses_count (ISR increment
site and _update_tachometer_speed read/reset) to use the atomic API so
reads/writes are safe and ordered.
- Around line 26-28: float_map can divide by zero when in_min == in_max; fix by
checking for that case (use a floating-point tolerant check like fabs(in_max -
in_min) < FLT_EPSILON) at the start of float_map and return a sensible fallback
(e.g., the midpoint of the output range (out_min+out_max)/2 or out_min) instead
of performing the division; keep the rest of the mapping logic unchanged so
float_map returns the mapped value when the denominator is non-zero.
- Around line 126-130: The destructor ~KobaEGPUUserModCore currently disables
the wrong GPIO (GPIO_NUM_8) while the tachometer ISR was installed on
FAN_TACHOMETER_PIN; update the destructor to call
gpio_isr_handler_remove(FAN_TACHOMETER_PIN) and then
gpio_intr_disable(FAN_TACHOMETER_PIN) (instead of GPIO_NUM_8) to properly remove
the ISR and disable interrupts, keeping the existing delete _adc_chars handling
intact.
- Around line 65-70: The voltage-to-resistance math in _calc_temp is using
inconsistent reference voltages (3.0f for voltage calculation and 3.3f in the
divider), causing wrong resistance/temperature; update the voltage computation
to use the same ADC reference as the divider (replace 3.0f with 3.3f or better,
centralize into a REF_VOLTAGE constant) so voltage = (float(adc_value) /
4095.0f) * REF_VOLTAGE and then compute rt = 10000.0f * (voltage / (REF_VOLTAGE
- voltage)); keep the bounds check and return via
_thermistor.calculateTemperatureFromResistance(rt).
- Around line 182-183: The call to gpio_install_isr_service(0) can return
ESP_ERR_INVALID_STATE if another usermod already installed the ISR service;
replace the unconditional ESP_ERROR_CHECK(gpio_install_isr_service(0)) with a
guarded check: call gpio_install_isr_service(0) into an esp_err_t variable, and
only treat it as an error if it's neither ESP_OK nor ESP_ERR_INVALID_STATE
(i.e., ignore ESP_ERR_INVALID_STATE), then proceed to call
gpio_isr_handler_add(gpio_num_t(FAN_TACHOMETER_PIN), tachometer_isr, this) as
before; keep references to gpio_install_isr_service, ESP_ERROR_CHECK,
gpio_isr_handler_add, FAN_TACHOMETER_PIN, tachometer_isr, and this so the change
is localized and clear.
- Around line 328-331: The string constants KobaEGPUUsermod::_gpu_temp_string,
KobaEGPUUsermod::_fan_temp_string,
KobaEGPUUsermod::_fan_min_speed_threshold_temperature_key, and
KobaEGPUUsermod::_fan_max_speed_threshold_temperature_key are missing the
PROGMEM attribute; update their declarations to store them in flash (add PROGMEM
to each const char array declaration to match the other strings in this class)
so they are defined like the other PROGMEM strings in KobaEGPUUsermod.
🧹 Nitpick comments (2)
usermods/KOBA_EGPU/koba_egpu.cpp (2)
7-21: Overly restrictive pin requirements reduce portability.The preprocessor errors require specific GPIO numbers (0, 1, 6, 10), which makes this usermod inflexible for users with different hardware configurations. Consider making the pins configurable at compile time without requiring exact values, or documenting why these specific pins are mandatory (e.g., ADC channel constraints).
♻️ Suggested change to allow any valid pin
-#if not defined(THERMISTOR_GPU_PIN) or THERMISTOR_GPU_PIN not_eq 0 -#error THERMISTOR_GPU_PIN NOT defined or not equals GPIO0 +#if not defined(THERMISTOR_GPU_PIN) +#error THERMISTOR_GPU_PIN NOT defined `#endif` -#if not defined(THERMISTOR_FAN_PIN) or THERMISTOR_FAN_PIN not_eq 1 -#error THERMISTOR_FAN_PIN NOT defined or not equals GPIO1 +#if not defined(THERMISTOR_FAN_PIN) +#error THERMISTOR_FAN_PIN NOT defined `#endif` -#if not defined(FAN_TACHOMETER_PIN) or FAN_TACHOMETER_PIN not_eq 6 +#if not defined(FAN_TACHOMETER_PIN) `#error` FAN_TACHOMETER_PIN NOT defined `#endif` -#if not defined(FAN_PWM_PIN) or FAN_PWM_PIN not_eq 10 +#if not defined(FAN_PWM_PIN) `#error` FAN_PWM_PIN NOT defined `#endif`
175-176: ADC calibration data allocated but never used.
_adc_charsis populated viaesp_adc_cal_characterizebut the code usesadc1_get_raw()directly instead ofesp_adc_cal_raw_to_voltage(). Either use the calibration for more accurate readings or remove the unused allocation.♻️ Option 1: Use calibration for better accuracy
float _calc_temp(int adc_value) { if (adc_value > 4000 or adc_value < 100) return NAN; - float voltage = (static_cast<float>(adc_value) / 4095.0f) * 3.0f; + uint32_t voltage_mv = esp_adc_cal_raw_to_voltage(adc_value, _adc_chars); + float voltage = voltage_mv / 1000.0f; float rt = 10000.0f * (voltage / (3.3f - voltage)); return _thermistor.calculateTemperatureFromResistance(rt); }
Comment on lines
+26
to
+28
| float float_map(float x, float in_min, float in_max, float out_min, float out_max) { | ||
| return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min; | ||
| } |
Contributor
There was a problem hiding this comment.
Potential division by zero if in_min == in_max.
While current usage validates thresholds, this helper could cause undefined behavior if called with equal min/max values.
🛡️ Defensive fix
float float_map(float x, float in_min, float in_max, float out_min, float out_max) {
+ if (in_max == in_min) return out_min;
return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
}🤖 Prompt for AI Agents
In `@usermods/KOBA_EGPU/koba_egpu.cpp` around lines 26 - 28, float_map can divide
by zero when in_min == in_max; fix by checking for that case (use a
floating-point tolerant check like fabs(in_max - in_min) < FLT_EPSILON) at the
start of float_map and return a sensible fallback (e.g., the midpoint of the
output range (out_min+out_max)/2 or out_min) instead of performing the division;
keep the rest of the mapping logic unchanged so float_map returns the mapped
value when the denominator is non-zero.
Member
There was a problem hiding this comment.
proposed fix:
if (in_min == in_max) return out_min;
else return ....
Contributor
There was a problem hiding this comment.
@softhack007, perfect! That fix looks good and will properly handle the edge case where in_min == in_max. Returning out_min in that scenario is a sensible fallback behavior. 👍
🧠 Learnings used
Learnt from: DedeHai
Repo: wled/WLED PR: 4456
File: usermods/deep_sleep/deep_sleep.cpp:224-230
Timestamp: 2025-11-30T15:29:00.726Z
Learning: In WLED, `touchThreshold` is a global variable defined in wled00/wled.h and is accessible to all usermods. It stores the touch sensitivity threshold for touch-capable GPIO pins on ESP32 variants and should not be flagged as undefined when used in usermods.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
| private: | ||
| bool _configured = false; | ||
| uint32_t _delta_update_time_ms = 100; | ||
| uint32_t _pulses_count = 0; |
Contributor
There was a problem hiding this comment.
Race condition on _pulses_count between ISR and main thread.
_pulses_count is modified by the ISR (line 120) and read/reset by _update_tachometer_speed (lines 106-108) without synchronization. This can cause lost pulse counts or corrupted reads.
🔒 Proposed fix using atomic operations
+#include <atomic>
+
namespace {
// ...
class KobaEGPUUserModCore {
private:
bool _configured = false;
uint32_t _delta_update_time_ms = 100;
- uint32_t _pulses_count = 0;
+ std::atomic<uint32_t> _pulses_count{0};
// ...
void _update_tachometer_speed(uint32_t delta_ms) {
- float new_rpm = float(_pulses_count) * 60.0f * 1000.0f / float(delta_ms);
+ uint32_t pulses = _pulses_count.exchange(0);
+ float new_rpm = float(pulses) * 60.0f * 1000.0f / float(delta_ms);
_last_check_tachometer_speed = millis();
- _pulses_count = 0;
if (not isnan(new_rpm) and new_rpm not_eq _current_rpm) {
_current_rpm = new_rpm;
}
}
IRAM_ATTR static void tachometer_isr(void *ctx) {
- reinterpret_cast<KobaEGPUUserModCore *>(ctx)->_pulses_count++;
+ reinterpret_cast<KobaEGPUUserModCore *>(ctx)->_pulses_count.fetch_add(1, std::memory_order_relaxed);
}Also applies to: 105-112, 119-121
🤖 Prompt for AI Agents
In `@usermods/KOBA_EGPU/koba_egpu.cpp` at line 55, _pulses_count is subject to a
race between the ISR and _update_tachometer_speed; change its type to
std::atomic<uint32_t> (include <atomic>) and replace non-atomic reads/resets
with atomic operations (e.g. use fetch_exchange(0) or exchange(0) in
_update_tachometer_speed to atomically capture-and-clear the count) while
keeping the ISR increment using fetch_add(1) (or ++ on the atomic). Update all
references to _pulses_count (ISR increment site and _update_tachometer_speed
read/reset) to use the atomic API so reads/writes are safe and ordered.
Comment on lines
+65
to
+70
| float _calc_temp(int adc_value) { | ||
| if (adc_value > 4000 or adc_value < 100) return NAN; | ||
| float voltage = (static_cast<float>(adc_value) / 4095.0f) * 3.0f; | ||
| float rt = 10000.0f * (voltage / (3.3f - voltage)); | ||
| return _thermistor.calculateTemperatureFromResistance(rt); | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistent reference voltage (3.0V vs 3.3V) causes incorrect temperature calculation.
Line 67 calculates voltage using 3.0V reference, but line 68 uses 3.3V for the voltage divider formula. This inconsistency will produce incorrect resistance and temperature values.
🐛 Proposed fix
float _calc_temp(int adc_value) {
if (adc_value > 4000 or adc_value < 100) return NAN;
- float voltage = (static_cast<float>(adc_value) / 4095.0f) * 3.0f;
- float rt = 10000.0f * (voltage / (3.3f - voltage));
+ constexpr float vref = 3.3f;
+ float voltage = (static_cast<float>(adc_value) / 4095.0f) * vref;
+ float rt = 10000.0f * (voltage / (vref - voltage));
return _thermistor.calculateTemperatureFromResistance(rt);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| float _calc_temp(int adc_value) { | |
| if (adc_value > 4000 or adc_value < 100) return NAN; | |
| float voltage = (static_cast<float>(adc_value) / 4095.0f) * 3.0f; | |
| float rt = 10000.0f * (voltage / (3.3f - voltage)); | |
| return _thermistor.calculateTemperatureFromResistance(rt); | |
| } | |
| float _calc_temp(int adc_value) { | |
| if (adc_value > 4000 or adc_value < 100) return NAN; | |
| constexpr float vref = 3.3f; | |
| float voltage = (static_cast<float>(adc_value) / 4095.0f) * vref; | |
| float rt = 10000.0f * (voltage / (vref - voltage)); | |
| return _thermistor.calculateTemperatureFromResistance(rt); | |
| } |
🤖 Prompt for AI Agents
In `@usermods/KOBA_EGPU/koba_egpu.cpp` around lines 65 - 70, The
voltage-to-resistance math in _calc_temp is using inconsistent reference
voltages (3.0f for voltage calculation and 3.3f in the divider), causing wrong
resistance/temperature; update the voltage computation to use the same ADC
reference as the divider (replace 3.0f with 3.3f or better, centralize into a
REF_VOLTAGE constant) so voltage = (float(adc_value) / 4095.0f) * REF_VOLTAGE
and then compute rt = 10000.0f * (voltage / (REF_VOLTAGE - voltage)); keep the
bounds check and return via _thermistor.calculateTemperatureFromResistance(rt).
Comment on lines
+126
to
+130
| ~KobaEGPUUserModCore() { | ||
| // todo destroy adc | ||
| delete _adc_chars; | ||
| gpio_intr_disable(GPIO_NUM_8); | ||
| } |
Contributor
There was a problem hiding this comment.
Destructor disables wrong GPIO pin.
gpio_intr_disable(GPIO_NUM_8) is hardcoded, but the tachometer ISR was installed on FAN_TACHOMETER_PIN (GPIO 6). The ISR on the correct pin remains active after destruction.
🐛 Proposed fix
~KobaEGPUUserModCore() {
// todo destroy adc
delete _adc_chars;
- gpio_intr_disable(GPIO_NUM_8);
+ gpio_isr_handler_remove(gpio_num_t(FAN_TACHOMETER_PIN));
+ gpio_intr_disable(gpio_num_t(FAN_TACHOMETER_PIN));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ~KobaEGPUUserModCore() { | |
| // todo destroy adc | |
| delete _adc_chars; | |
| gpio_intr_disable(GPIO_NUM_8); | |
| } | |
| ~KobaEGPUUserModCore() { | |
| // todo destroy adc | |
| delete _adc_chars; | |
| gpio_isr_handler_remove(gpio_num_t(FAN_TACHOMETER_PIN)); | |
| gpio_intr_disable(gpio_num_t(FAN_TACHOMETER_PIN)); | |
| } |
🤖 Prompt for AI Agents
In `@usermods/KOBA_EGPU/koba_egpu.cpp` around lines 126 - 130, The destructor
~KobaEGPUUserModCore currently disables the wrong GPIO (GPIO_NUM_8) while the
tachometer ISR was installed on FAN_TACHOMETER_PIN; update the destructor to
call gpio_isr_handler_remove(FAN_TACHOMETER_PIN) and then
gpio_intr_disable(FAN_TACHOMETER_PIN) (instead of GPIO_NUM_8) to properly remove
the ISR and disable interrupts, keeping the existing delete _adc_chars handling
intact.
Comment on lines
+182
to
+183
| ESP_ERROR_CHECK(gpio_install_isr_service(0)); | ||
| ESP_ERROR_CHECK(gpio_isr_handler_add(gpio_num_t(FAN_TACHOMETER_PIN), tachometer_isr, this)); |
Contributor
There was a problem hiding this comment.
gpio_install_isr_service may abort if already installed by another usermod.
If another usermod has already installed the ISR service, this call returns ESP_ERR_INVALID_STATE and ESP_ERROR_CHECK will abort. Handle this case gracefully.
🛡️ Proposed fix
- ESP_ERROR_CHECK(gpio_install_isr_service(0));
+ esp_err_t err = gpio_install_isr_service(0);
+ if (err != ESP_OK && err != ESP_ERR_INVALID_STATE) {
+ ESP_ERROR_CHECK(err); // Will abort on unexpected errors
+ }
ESP_ERROR_CHECK(gpio_isr_handler_add(gpio_num_t(FAN_TACHOMETER_PIN), tachometer_isr, this));🤖 Prompt for AI Agents
In `@usermods/KOBA_EGPU/koba_egpu.cpp` around lines 182 - 183, The call to
gpio_install_isr_service(0) can return ESP_ERR_INVALID_STATE if another usermod
already installed the ISR service; replace the unconditional
ESP_ERROR_CHECK(gpio_install_isr_service(0)) with a guarded check: call
gpio_install_isr_service(0) into an esp_err_t variable, and only treat it as an
error if it's neither ESP_OK nor ESP_ERR_INVALID_STATE (i.e., ignore
ESP_ERR_INVALID_STATE), then proceed to call
gpio_isr_handler_add(gpio_num_t(FAN_TACHOMETER_PIN), tachometer_isr, this) as
before; keep references to gpio_install_isr_service, ESP_ERROR_CHECK,
gpio_isr_handler_add, FAN_TACHOMETER_PIN, tachometer_isr, and this so the change
is localized and clear.
Comment on lines
+328
to
+331
| const char KobaEGPUUsermod::_gpu_temp_string[] = "GPU Temp"; | ||
| const char KobaEGPUUsermod::_fan_temp_string[] = "FAN Temp"; | ||
| const char KobaEGPUUsermod::_fan_min_speed_threshold_temperature_key[] = "Fan min speed threshold temperature (C)"; | ||
| const char KobaEGPUUsermod::_fan_max_speed_threshold_temperature_key[] = "Fan max speed threshold temperature (C)"; |
Contributor
There was a problem hiding this comment.
Missing PROGMEM on string constants wastes RAM.
These strings are missing PROGMEM unlike the others above, causing them to be stored in RAM instead of flash.
♻️ Add PROGMEM for consistency
-const char KobaEGPUUsermod::_gpu_temp_string[] = "GPU Temp";
-const char KobaEGPUUsermod::_fan_temp_string[] = "FAN Temp";
-const char KobaEGPUUsermod::_fan_min_speed_threshold_temperature_key[] = "Fan min speed threshold temperature (C)";
-const char KobaEGPUUsermod::_fan_max_speed_threshold_temperature_key[] = "Fan max speed threshold temperature (C)";
+const char KobaEGPUUsermod::_gpu_temp_string[] PROGMEM = "GPU Temp";
+const char KobaEGPUUsermod::_fan_temp_string[] PROGMEM = "FAN Temp";
+const char KobaEGPUUsermod::_fan_min_speed_threshold_temperature_key[] PROGMEM = "Fan min speed threshold temperature (C)";
+const char KobaEGPUUsermod::_fan_max_speed_threshold_temperature_key[] PROGMEM = "Fan max speed threshold temperature (C)";🤖 Prompt for AI Agents
In `@usermods/KOBA_EGPU/koba_egpu.cpp` around lines 328 - 331, The string
constants KobaEGPUUsermod::_gpu_temp_string, KobaEGPUUsermod::_fan_temp_string,
KobaEGPUUsermod::_fan_min_speed_threshold_temperature_key, and
KobaEGPUUsermod::_fan_max_speed_threshold_temperature_key are missing the
PROGMEM attribute; update their declarations to store them in flash (add PROGMEM
to each const char array declaration to match the other strings in this class)
so they are defined like the other PROGMEM strings in KobaEGPUUsermod.
Collaborator
|
This is very highly application specific and has to be an out of tree usermod. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The mod does not make significant changes to the WLED system, but only supplements it with an additional, optional mod. If we describe the mod most compactly, then it is intended for a self-made eGPU station.
Summary by CodeRabbit