added new usermod adc_sensor_mqtt_v2 with hass integration #4635
Conversation
WalkthroughThis change introduces a new usermod called Changes
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
usermods/photoresistor_sensor_mqtt_v2/README.md (7)
1-1: Fix spelling of “Photoresister” heading.
To maintain consistent terminology:- # Photoresister sensor with MQTT ## + # Photoresistor sensor with MQTT ##
2-2: Add the article "an" before 'auto discovery message'.- it will publish auto discovery message to hass mqtt + it will publish an auto-discovery message to Hass MQTT🧰 Tools
🪛 LanguageTool
[uncategorized] ~2-~2: You might be missing the article “an” here.
Context: ... more hass application. it will publish auto discovery message to hass mqtt and atta...(AI_EN_LECTOR_MISSING_DETERMINER_AN)
8-8: Specify a language for the fenced code block.
Adding a language identifier (e.g.,bash,ini, orplaintext) to fenced code blocks improves readability and adheres to markdownlint guidelines.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
8-8: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
16-16: Surround the table with blank lines.
Markdown guidelines recommend blank lines before and after tables. Adding these will clear the markdownlint warning.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
16-16: Tables should be surrounded by blank lines
null(MD058, blanks-around-tables)
28-28: Convert bare URL to a proper link.- [@jeffwdh](https://github.com/jeffwdh) - [email protected] + [@jeffwdh](https://github.com/jeffwdh) + [mailto:[email protected]]([email protected])Or simply enclose the email in angled brackets:
<[email protected]>.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
28-28: Bare URL used
null(MD034, no-bare-urls)
31-31: Add a comma before “so” to fix run-on sentence.- A threshold value can be set so significant changes... + A threshold value can be set, so significant changes...🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... definable. A threshold value can be set so significant changes in the readings are...(COMMA_COMPOUND_SENTENCE_2)
35-35: Add a missing comma after 'sensor'.- This usermod has only been tested with the KY-018 sensor though... + This usermod has only been tested with the KY-018 sensor, though...🧰 Tools
🪛 LanguageTool
[uncategorized] ~35-~35: Possible missing comma found.
Context: ...od has only been tested with the KY-018 sensor though it should work for any other ana...(AI_HYDRA_LEO_MISSING_COMMA)
usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp (2)
7-7: Correct the spelling of “threshould”.- float change_threshould = 1; + float change_threshold = 1;
57-57: Fix variable name “force_upadate”.- bool force_upadate = ... + bool force_update = ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.gitignore(1 hunks)usermods/PIR_sensor_switch/PIR_sensor_switch.cpp(1 hunks)usermods/photoresistor_sensor_mqtt_v2/README.md(1 hunks)usermods/photoresistor_sensor_mqtt_v2/library.json(1 hunks)usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp(1 hunks)wled00/FX_fcn.cpp(1 hunks)wled00/FXparticleSystem.cpp(0 hunks)wled00/const.h(1 hunks)wled00/network.cpp(1 hunks)wled00/pin_manager.h(1 hunks)wled00/wled.cpp(1 hunks)wled00/wled.h(1 hunks)wled00/xml.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- wled00/FXparticleSystem.cpp
🧰 Additional context used
🧬 Code Graph Analysis (4)
wled00/const.h (1)
usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp (1)
USERMOD_PHOTORESISTOR_MQTT_V2(144-147)
wled00/pin_manager.h (2)
usermods/pixels_dice_tray/pixels_dice_tray.cpp (1)
USERMOD_ID_PIXELS_DICE_TRAY(528-528)usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp (1)
USERMOD_PHOTORESISTOR_MQTT_V2(144-147)
wled00/network.cpp (1)
wled00/FX.h (1)
selected(591-591)
usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp (2)
wled00/pin_manager.h (4)
allocatePin(89-89)allocatePin(97-97)deallocatePin(83-83)deallocatePin(99-99)usermods/SN_Photoresistor/SN_Photoresistor.cpp (1)
REGISTER_USERMOD(214-214)
🪛 LanguageTool
usermods/photoresistor_sensor_mqtt_v2/README.md
[uncategorized] ~2-~2: You might be missing the article “an” here.
Context: ... more hass application. it will publish auto discovery message to hass mqtt and atta...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
[uncategorized] ~31-~31: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... definable. A threshold value can be set so significant changes in the readings are...
(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~35-~35: Possible missing comma found.
Context: ...od has only been tested with the KY-018 sensor though it should work for any other ana...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
usermods/photoresistor_sensor_mqtt_v2/README.md
8-8: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
16-16: Tables should be surrounded by blank lines
null
(MD058, blanks-around-tables)
28-28: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (10)
.gitignore (1)
26-26: LGTM! Added build cache to gitignore.Adding
.buildcacheto.gitignoreis good practice to prevent local build artifacts from being committed to the repository.wled00/FX_fcn.cpp (1)
1498-1498: Good type safety improvementChanging the loop iterator from
inttosize_timproves type safety by ensuring the variable used for indexing is an unsigned type, which aligns better with the return type ofBusManager::getNumBusses().wled00/const.h (1)
213-213: LGTM! Added new usermod identifierThe new usermod identifier
USERMOD_PHOTORESISTOR_MQTT_V2follows the correct naming convention and is assigned the next available ID (58).wled00/wled.h (1)
370-370: Improved type safety with size_t for array indexChanged variable type from
int8_ttosize_twhich is more appropriate for indexing into the WiFi networks vector. This is a good practice as it:
- Provides better type safety for array indices
- Allows for more than 127 WiFi networks (the limit of int8_t)
- Matches the standard C++ container indexing type
wled00/wled.cpp (1)
753-753: Suppressed compiler warning for potentially unused variableAdded
__attribute__((unused))to thenowSvariable to prevent compiler warnings when the variable is not used in certain code paths. This is a clean approach to maintain the variable for debugging or future use while eliminating unwanted warnings.usermods/PIR_sensor_switch/PIR_sensor_switch.cpp (1)
320-322: Enhanced Home Assistant integration with device identificationAdded device connection information to the Home Assistant MQTT auto-discovery message. This improves device identification in the Home Assistant ecosystem by providing the MAC address as a unique identifier.
This change follows the Home Assistant discovery protocol standards and will make device management more reliable in Home Assistant.
wled00/xml.cpp (1)
298-298: Improved type safety with size_t for bus iterationChanged loop counter from
inttosize_tfor better type safety when iterating over buses. This is consistent with the C++ standard practice of usingsize_tfor container indices and counters, reducing the risk of negative values or overflow.wled00/network.cpp (1)
299-299: Correct use ofsize_tfor indexing.
This change frominttosize_tis appropriate for indexing and helps avoid potential out-of-range issues.wled00/pin_manager.h (1)
76-77: Addition of new usermod enumerator looks good.
The newUM_PHOTO_RESISTOR_MQTTenumerator aligns well with the newly introduced usermod.usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp (1)
57-57: Verify large multiplier for forced update frequency.
The expression(update_interval * 100)introduces a 100× factor, possibly causing forced updates at a much slower rate than expected. Confirm that a 5-minute interval (3000ms × 100) is intended.
usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp (4)
6-6: Consider using unsigned long for timing calculations
In Arduino,millis()returns anunsigned long. Using a signed type likelongforlastTimeandupdate_intervalcan lead to sign-comparison issues or overflow problems over extended run times.- long update_interval = 3000; - long lastTime = 0; + unsigned long update_interval = 3000UL; + unsigned long lastTime = 0UL;Also applies to: 10-10, 55-58
7-7: Fix spelling of “threshold”
Your code uses the variable and key"change_threshould"in multiple places. Renaming them to"change_threshold"improves clarity and avoids confusion.- float change_threshould = 1; + float change_threshold = 1; - top["change threshould"] = change_threshould; + top["change threshold"] = change_threshold; - configComplete &= getJsonValue(top["change threshould"], change_threshould); + configComplete &= getJsonValue(top["change threshold"], change_threshold); - JsonArray LDR_change_threshould = user.createNestedArray("change threshould"); + JsonArray LDR_change_threshold = user.createNestedArray("change threshold");Also applies to: 100-100, 112-112, 140-140
16-16: Simplify inverted logic for readability
The current arithmetic with* -1works but can be confusing. Splitting out the expression clarifies the behavior, especially when inverted istrue.- if (inverted) - { - lightPercentage = 100.0 - ((float)lightValue * -1 + mapping) / (float)mapping * 100; - } - else - { - lightPercentage = ((float)lightValue * -1 + mapping) / (float)mapping * 100; - } + float normalized = (mapping - (float)lightValue) / mapping * 100.0; + if (inverted) { + lightPercentage = 100.0 - normalized; + } else { + lightPercentage = normalized; + }Also applies to: 69-73
57-57: Rename “force_upadate” to “force_update”
There is a minor spelling issue in the variable name. Renaming it makes the code clearer.- bool force_upadate = (millis() - lastTime) > (update_interval * 100); + bool force_update = (millis() - lastTime) > (update_interval * 100);Also applies to: 76-76
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp(1 hunks)
🔇 Additional comments (1)
usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp (1)
187-187: Confirm hardware version labeling for ESP8266
Here,device[F("hw_version")] = F("esp32");suggests the hardware is always ESP32, but there is partial ESP8266 support. Verify if this is intentional.Please confirm your intended platforms; if you also support ESP8266, consider setting this dynamically or via a compile-time macro.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp (4)
24-43: Consider logging warnings for invalid pin scenarios.
At present, the usermod is silently disabled when an invalid or busy pin is detected. Logging a warning or error message would help with debugging and user feedback.if (!PinManager::allocatePin(ldrPin, false, PinOwner::UM_PHOTO_RESISTOR_MQTT)) { ldrEnabled = false; // pin already in use -> disable usermod + DEBUG_PRINTLN(F("photoresistor_sensor_mqtt_v2: Pin already in use, disabling usermod.")); } else { pinMode(ldrPin, INPUT); // alloc success -> configure pin for input } } else { ldrEnabled = false; // invalid pin -> disable usermod + DEBUG_PRINTLN(F("photoresistor_sensor_mqtt_v2: Invalid pin, disabling usermod.")); }
45-51: Avoid triggering Home Assistant discovery multiple times.
Since discovery is also published in thepublishMqtt()method whenhassDiscoverySentis false, callingpublishDiscovery()again inconnected()may result in redundant messages if the function is called more than once.void connected() { if (WLED_MQTT_CONNECTED && HomeAssistantDiscovery + && !hassDiscoverySent ) { publishDiscovery(); } }
53-91: Use dedicated floating-point absolute function.
abs()is overloaded in C++, but it's clearer to usestd::fabs()orfabs()for floating-point arguments to avoid any ambiguity.- if (abs(lightPercentage - lastPercentage) > change_threshold || force_update) + if (fabs(lightPercentage - lastPercentage) > change_threshold || force_update)
93-102: Fix the repeated typo "threshould" → "threshold".--- a/usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp +++ b/usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp @@ -100 +100 @@ - top["change threshould"] = change_threshold; + top["change threshold"] = change_threshold; @@ -112 +112 @@ - configComplete &= getJsonValue(top["change threshould"], change_threshold); + configComplete &= getJsonValue(top["change threshold"], change_threshold); @@ -140 +140 @@ - JsonArray LDR_change_threshold = user.createNestedArray("change threshould"); + JsonArray LDR_change_threshold = user.createNestedArray("change threshold");Also applies to: 103-120, 122-142
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp (1)
wled00/pin_manager.h (4)
allocatePin(89-89)allocatePin(97-97)deallocatePin(83-83)deallocatePin(99-99)
🔇 Additional comments (2)
usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp (2)
1-3: No concerns with this initial class definition.
203-206: Looks good.
| void publishMqtt(float state) | ||
| { | ||
| if (WLED_MQTT_CONNECTED) | ||
| { | ||
| if (!hassDiscoverySent && HomeAssistantDiscovery) | ||
| { | ||
| publishDiscovery(); | ||
| } | ||
| char subuf[38]; | ||
| strcpy(subuf, mqttDeviceTopic); | ||
| strcat(subuf, MQTT_TOPIC); | ||
| mqtt->publish(subuf, 0, true, String(state).c_str()); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify maximum lengths to prevent buffer overflow.
subuf, uid, and buf have fixed sizes, but they rely on mqttDeviceTopic and MQTT_TOPIC (or uid for the discovery path) not exceeding certain lengths. If these strings are longer than expected, a buffer overflow could occur.
Also applies to: 164-201
🏁 Script executed:
#!/bin/bash
# Description: Validate the maximum length constraints for mqttDeviceTopic, escapedMac, and other strings.
# 1. Search for definitions or references of mqttDeviceTopic and escapedMac to check any enforced size.
rg -A 5 "mqttDeviceTopic|escapedMac"
# 2. Inspect how these variables are assigned or constrained in the codebase.
# If there's no size limit, consider using safer functions like strlcpy or strncat.Length of output: 51656
Potential Buffer Overflow in MQTT Publish Function – Use Safe String Functions
The use of strcpy and strcat in the publishMqtt() function (lines 149–162 in usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp) is risky if the lengths of mqttDeviceTopic and MQTT_TOPIC exceed the expected limits. Our grep output shows that many parts of the codebase already enforce size limits using safe functions like strlcpy, strlcat, or snprintf_P. To mitigate a potential buffer overflow:
- Replace
strcpy/strcatwith safe variants (e.g.,strlcpy/strlcator equivalent) using the correct size (e.g.,MQTT_MAX_TOPIC_LEN + 1for the topic buffer). - Verify that the fixed buffer (
subuf[38]) is sufficiently sized to holdmqttDeviceTopicplusMQTT_TOPIC. If not, adjust its size accordingly. - Consider checking the lengths of these strings prior to concatenation.
Please update the function to use safer string operations to avoid overflow risks.
|
thank you for contributing. |
… improved hass intgeration for PIR_sensor_switch
|
@DedeHai thanks for the review ,i rebased it to have the UM as a single commit. tested on esp32 & esp8266 nodmcu & esp_01 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
usermods/photoresistor_sensor_mqtt_v2/README.md (5)
8-13: Specify Language for Fenced Code Block
The fenced code block in the installation example does not specify a language, which is flagged by markdownlint (MD040). Adding a language specifier such asinican improve readability and tooling support. For example, change the opening fence from:-``` +```iniThis small change helps keep the documentation aligned with best practices.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
8-8: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
15-23: Improve Table Formatting for Better Markdown Readability
The table under the "Usermod Settings" header would benefit from having blank lines both before and after it to conform with markdown formatting guidelines (MD058). For instance, insert a blank line immediately before line 16 and after line 23.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
16-16: Tables should be surrounded by blank lines
null(MD058, blanks-around-tables)
24-29: Enhance Author Section Formatting
Consider revising the "Author" section for consistent formatting. For example:
- Capitalize “Based on:” on line 26.
- Convert the bare email address on line 28 to a proper mailto link (e.g.,
<[email protected]>or[[email protected]](mailto:[email protected])) to avoid markdownlint warnings (MD034).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
28-28: Bare URL used
null(MD034, no-bare-urls)
31-33: Refine Messaging and Punctuation for Clarity
The sentence on line 31 would be clearer with improved punctuation and phrasing. For example, change:"A threshold value can be set so significant changes in the readings are published immediately vice waiting for the next update."
to
"A threshold value can be set, so that significant changes in the readings are published immediately rather than waiting for the next update."
Additionally, consider revising the sentence on line 33 to improve its flow; for instance, "I also found it useful to limit the frequency of analog pin reads; otherwise, the board hangs."
🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... definable. A threshold value can be set so significant changes in the readings are...(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~31-~31: Possible missing comma found.
Context: ...t changes in the readings are published immediately vice waiting for the next update. This ...(AI_HYDRA_LEO_MISSING_COMMA)
35-36: Minor Punctuation and Clarity Improvements in Usermod Testing Note
On line 35, adding a comma after "KY-018 sensor" can improve readability:"This usermod has only been tested with the KY-018 sensor, though it should work for any other analog pin sensor."
Similarly, on line 36, consider rephrasing to enhance clarity—for example:
"Note: This usermod does not directly control the LED strip; it only publishes MQTT readings for use with integrations like Home Assistant."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitignore(1 hunks)usermods/PIR_sensor_switch/PIR_sensor_switch.cpp(1 hunks)usermods/photoresistor_sensor_mqtt_v2/README.md(1 hunks)usermods/photoresistor_sensor_mqtt_v2/library.json(1 hunks)usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp(1 hunks)wled00/FXparticleSystem.cpp(0 hunks)wled00/const.h(1 hunks)wled00/pin_manager.h(1 hunks)
💤 Files with no reviewable changes (1)
- wled00/FXparticleSystem.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- usermods/photoresistor_sensor_mqtt_v2/library.json
- .gitignore
- wled00/const.h
- usermods/PIR_sensor_switch/PIR_sensor_switch.cpp
- wled00/pin_manager.h
- usermods/photoresistor_sensor_mqtt_v2/photoresistor_sensor_mqtt_v2.cpp
🧰 Additional context used
🪛 LanguageTool
usermods/photoresistor_sensor_mqtt_v2/README.md
[uncategorized] ~2-~2: You might be missing the article “an” here.
Context: ... more hass application. it will publish auto discovery message to hass mqtt and atta...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
[uncategorized] ~31-~31: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... definable. A threshold value can be set so significant changes in the readings are...
(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~31-~31: Possible missing comma found.
Context: ...t changes in the readings are published immediately vice waiting for the next update. This ...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
usermods/photoresistor_sensor_mqtt_v2/README.md
8-8: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
16-16: Tables should be surrounded by blank lines
null
(MD058, blanks-around-tables)
28-28: Bare URL used
null
(MD034, no-bare-urls)
| # Photoresister sensor with MQTT ## based on LDR_Dusk_Dawn_v2 usermod from [@jeffwdh](https://github.com/jeffwdh) | ||
| This usermod will obtain readings from a Light Dependent Resistor (LDR). This is useful for exterior lighting situations where you want the lights to only be on when it is dark out. but this mod is designed for more hass application. it will publish auto discovery message to hass mqtt and attach the light sensor to the wled integration automatically |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify and Correct the Introduction
The header and opening paragraph contain a few typographical and grammatical issues. For example, "Photoresister" should be corrected to "Photoresistor" to match the file name, and the wording—especially around "more hass application" and "auto discovery message"—can be improved for clarity and formality. Consider revising the text as follows:
"Photoresistor sensor with MQTT – based on the LDR_Dusk_Dawn_v2 usermod from @jeffwdh"
This usermod obtains readings from a Light Dependent Resistor (LDR), which is useful for exterior lighting conditions where the lights should only be on when it is dark. However, this mod is designed primarily for Home Assistant integration: it publishes auto-discovery messages via MQTT to automatically attach the light sensor to the WLED integration.
This will address the issues flagged by LanguageTool and improve overall clarity.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~2-~2: You might be missing the article “an” here.
Context: ... more hass application. it will publish auto discovery message to hass mqtt and atta...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
|
please dont force push. there are still changed files not part of the UM |
|
alright. |
|
no, you still mixed two PRs and changed unrelated files. |
sorry , my bad. now should be good |
|
I took a closer look at the UM. If the purpose of it is to read an analog value and send it to MQTT why not just name it that? It is not limited to a photoresistor even if that is what you use it for. It would be much better suited as a general "read analog input, publish the value", it could even be extended to multiple pins. I did not check if any usermod already does something like this. |
it can be adapted as general purpose analog read, but the UM aims to support hass wled integration via mqqt as light sensor with auto discovery. so if its not used as light sensor, hass discovery needs adjustement. |
|
I am unfamiliar with how HA works. If you made this into a more generally useable UM, there could be any analog sensor attached. If HA uses a particular structure: adding a dropdown to the UM config to select what type HA should be detecting would be a good option. |
DedeHai
changed the title
|
I read the code some more.
@blazoncek since you are more familiar with UMs capabilities, would you care to take a look? |
| change threshould | the minimum limit to detect change and publish the value (in % ; float ; ex 0.8 ) . | 1.0 | ||
|
|
||
| ## Author | ||
| [@jeffrommo911wdh](https://github.com/rommo911) |
There was a problem hiding this comment.
i'll put it in a seperate PR . its very useful for hass integration
| JsonObject top = root[FPSTR(_name)]; | ||
| bool configComplete = !top.isNull(); | ||
| configComplete &= getJsonValue(top["Enabled"], ldrEnabled); | ||
| configComplete &= getJsonValue(top["LDR Pin"], ldrPin); |
There was a problem hiding this comment.
Even though it is (currently) supported, I would avoid spaces in JSON key names.
| bool configComplete = !top.isNull(); | ||
| configComplete &= getJsonValue(top["Enabled"], ldrEnabled); | ||
| configComplete &= getJsonValue(top["LDR Pin"], ldrPin); | ||
| configComplete &= getJsonValue(top["LDR update interval"], update_interval); |
There was a problem hiding this comment.
These are cfg.json configuration item key names. They need not be descriptive. Use addInfo() to add description.
| configComplete &= getJsonValue(top["change threshould"], change_threshold); | ||
| // pin changed - un-register previous pin, register new pin | ||
| PinManager::deallocatePin(oldLdrPin, PinOwner::UM_PHOTO_RESISTOR_MQTT); | ||
| if (ldrEnabled && ldrPin > 0) |
| static const char MQTT_TOPIC[]; | ||
| static const char _name[]; | ||
| const bool HomeAssistantDiscovery = true; // is HA discovery turned on by default | ||
| int ldrPin = A0; |
There was a problem hiding this comment.
Please use actual GPIO number and not Arduino pin monikers.
alrighty , i am still new to WLED PR. i use it very much around my house with several UM & hass integration ( i have almost all versions running in my house , ESP01 / ESP8266 / ESP32 / ESP32-S2 ESP32-S3 .
i am working on modifying it to be as @DedeHai , analog sensor with multiple pins support & hass general WLED integration to be most correct, current mqtt hass dicovery add a seperate mqtt device that is identified outside the WLED device in hass. this integration would put the mqtt entity inside the WLED device menu in hass. TODO : |
|
@blazoncek i have issue with choosing pin A0 in esp8266 via pinManager , actually A0 is pin number 17 . and pin manager refuses to allocate pin 17 as its ONLY ADC pin and in the code its explicitly specified to return false if pin >=17 for ESP8266 ... |
|
I think a more or less generic analog sensor UM with MQTT and proper HA support would be greatly appreciated by other users. There have been several requests in this direction. |
|
ESP8266 only has one (un-modifiable) ADC pin and is so pointless to allow selection of it. |
on it . |
…ort & sensor attribute support.
|
its looking like this at this moment
for ESP8266 , removed pin choice and forced A0=17 , ignore changes on pin. HASS detect the sensor class & its unit. TODO : gotta check with pin for ESP32-C3 / S2 / S3 to ensure appropriate ADC pins are allowed, would appreciate your feedback or advices while working on it to consider other aspects. spoiler alert : the UM setup / config / discovery is rather heavy complicated code but since its for ESP series , it can be handled quit good i guess. |
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
usermods/adc_sensor_mqtt/README.md (1)
16-23: 🛠️ Refactor suggestionUpdate and expand the settings table.
The settings table is missing several configuration options and should have blank lines around it for proper markdown formatting.
Apply these improvements to the settings table:
# Usermod Settings + Setting | Description | Default --- | --- | --- Enabled | Enable/Disable the LDR functionality. | Disabled - Pin | The analog capable pin your LDR is connected to. | A0 - update interval | update interval to read the analog pin ( in ms ) | 3000 - inverted | invert readings based on your own hardware setup ( invert mapping analog to digital value to 0V = 100% or 3.3V = 100% ) | false -change threshould | the minimum limit to detect change and publish the value (in % ; float ; ex 0.8 ) . | 1.0 + AdcPin_0 | The analog capable pin your sensor is connected to. (ESP32: 32-39, ESP8266: A0) | -1 (ESP32), A0 (ESP8266) + DeviceClass_0 | Home Assistant device class for the sensor. | illuminance + UnitOfMeas_0 | Home Assistant unit of measurement for the sensor. | lx + AdcUpdateInterval | Update interval to read the analog pin (in ms). | 2000 + AdcInverted | Invert readings based on your hardware setup (0V = 100% or 3.3V = 100%). | false + ChangeThreshold | The minimum change required to publish a new value (in %, float). | 1.0 + HomeAssistantDiscovery | Enable Home Assistant MQTT auto-discovery. | true + PublishRawValue | Publish raw ADC values instead of percentages. | false + +## Hardware Notes + +- For ESP8266, only the A0 pin (GPIO17) is available for analog input. +- For ESP32, pins 32-39 can be used for analog input. +- Connect your sensor (LDR, thermistor, etc.) with appropriate pull-up/pull-down resistors. + +## Home Assistant Integration + +This usermod will automatically create sensors in Home Assistant when Discovery is enabled. +The sensors will be grouped under your WLED device.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
16-16: Tables should be surrounded by blank lines
null(MD058, blanks-around-tables)
🧹 Nitpick comments (10)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp (7)
134-135: Improve debug message for ADC readings.The debug message is printing the percentage value as an 8-bit integer (0-255), which will truncate values and could cause confusion.
Apply this diff to fix the debug message:
- sprintf_P(buf, PSTR("adc %d: %d (%d)"), i, static_cast<uint8_t>(adc_Percentage[i]), adc_value[i]); + sprintf_P(buf, PSTR("adc %d: %.2f (%d)"), i, adc_Percentage[i], adc_value[i]);
174-174: Fix spelling error in configuration key.There's a spelling error in the configuration key "ChangeThreshould".
Apply this diff to fix the typo:
- top["ChangeThreshould"] = change_threshold; + top["ChangeThreshold"] = change_threshold;Make sure to update this in the readFromConfig method as well.
238-238: Fix spelling error in configuration key when reading.The spelling error in the configuration key "ChangeThreshould" is also present in the readFromConfig method.
Apply this diff to fix the typo:
- configComplete &= getJsonValue(top["ChangeThreshould"], change_threshold); + configComplete &= getJsonValue(top["ChangeThreshold"], change_threshold);
342-391: Consider adding MQTT entity categories for Home Assistant.Home Assistant discovery can be improved by adding entity categories to help organize the sensors in the UI.
Add entity category to the discovery message:
doc[F("dev_cla")] = device_class[pin_number].c_str(); doc[F("unit_of_meas")] = unit_of_meas[pin_number].c_str(); doc[F("val_tpl")] = F("{{ value }}"); + doc[F("ent_cat")] = F("diagnostic");
103-103: Reconsider the force update interval multiplier.Using a multiplier of 100 for the force update interval could lead to very long delays between forced updates.
Consider using a smaller multiplier or a fixed interval:
- bool force_update = (millis() - lastTime) > (update_interval * 100); // to keep the mqtt alive if the light is not changing + bool force_update = (millis() - lastTime) > (update_interval * 10); // to keep the mqtt alive if the light is not changing
46-47: Enhance default device class and unit settings.For a usermod that's primarily used with LDR sensors, consider using more appropriate defaults.
- device_class[i] = "voltage"; // default device class - unit_of_meas[i] = "V"; // default unit of measurement + device_class[i] = "illuminance"; // default device class for LDR + unit_of_meas[i] = "lx"; // default unit of measurement for light sensors
1-4: Consider adjusting the maximum number of pins.The default maximum of 1 ADC pin is quite limiting, especially for ESP32 which supports multiple ADC pins.
#include "wled.h" #ifndef UM_ADC_MQTT_PIN_MAX_NUMBER -#define UM_ADC_MQTT_PIN_MAX_NUMBER 1 +#define UM_ADC_MQTT_PIN_MAX_NUMBER 4 #endifusermods/adc_sensor_mqtt/README.md (3)
1-2: Improve README clarity and formatting.The introduction paragraph has grammar and punctuation issues that should be fixed for better readability.
Apply these improvements to the introduction:
-# ADC sensor with MQTT ## based on LDR_Dusk_Dawn_v2 usermod from [@jeffwdh](https://github.com/jeffwdh) -This usermod will obtain readings from adc pin. This is useful for ldr for example for exterior lighting situations where you want the lights to only be on when it is dark out. but this mod is designed for more hass application. it will publish auto discovery message to hass mqtt and attach the sensor to the wled integration automatically +# ADC Sensor with MQTT +## Based on LDR_Dusk_Dawn_v2 usermod from [@jeffwdh](https://github.com/jeffwdh) + +This usermod obtains readings from ADC pins. This is useful for LDR sensors, for example, for exterior lighting situations where you want the lights to only be on when it is dark out. The mod is designed for Home Assistant integration. It will publish an auto-discovery message to Home Assistant MQTT and attach the sensor to the WLED integration automatically.🧰 Tools
🪛 LanguageTool
[uncategorized] ~2-~2: A comma might be missing here.
Context: ...adings from adc pin. This is useful for ldr for example for exterior lighting situa...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~2-~2: A comma might be missing here.
Context: ...rom adc pin. This is useful for ldr for example for exterior lighting situations where ...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~2-~2: You might be missing the article “an” here.
Context: ... more hass application. it will publish auto discovery message to hass mqtt and atta...(AI_EN_LECTOR_MISSING_DETERMINER_AN)
8-14: Add language identifier to code block.The code block should specify a language for better syntax highlighting.
-``` +```ini [env:adc_sensor_mqtt_esp32dev] extends = env:esp32dev custom_usermods = ${env:esp32dev.custom_usermods} - adc_sensor_mqtt # Enable LDR Dusk Dawn Usermod + adc_sensor_mqtt # Enable ADC Sensor MQTT Usermod<details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 8-8: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> --- `12-13`: **Update comment in installation example.** The comment in the installation example refers to "LDR Dusk Dawn Usermod" instead of the current usermod name. ```diff custom_usermods = ${env:esp32dev.custom_usermods} - adc_sensor_mqtt # Enable LDR Dusk Dawn Usermod + adc_sensor_mqtt # Enable ADC Sensor MQTT Usermod
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
usermods/PIR_sensor_switch/PIR_sensor_switch.cpp(0 hunks)usermods/adc_sensor_mqtt/README.md(1 hunks)usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp(1 hunks)usermods/adc_sensor_mqtt/library.json(1 hunks)wled00/const.h(1 hunks)wled00/pin_manager.h(1 hunks)
💤 Files with no reviewable changes (1)
- usermods/PIR_sensor_switch/PIR_sensor_switch.cpp
✅ Files skipped from review due to trivial changes (1)
- usermods/adc_sensor_mqtt/library.json
🚧 Files skipped from review as they are similar to previous changes (2)
- wled00/const.h
- wled00/pin_manager.h
🧰 Additional context used
🪛 LanguageTool
usermods/adc_sensor_mqtt/README.md
[uncategorized] ~2-~2: A comma might be missing here.
Context: ...adings from adc pin. This is useful for ldr for example for exterior lighting situa...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~2-~2: A comma might be missing here.
Context: ...rom adc pin. This is useful for ldr for example for exterior lighting situations where ...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~2-~2: You might be missing the article “an” here.
Context: ... more hass application. it will publish auto discovery message to hass mqtt and atta...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
🪛 markdownlint-cli2 (0.17.2)
usermods/adc_sensor_mqtt/README.md
8-8: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
16-16: Tables should be surrounded by blank lines
null
(MD058, blanks-around-tables)
| public: | ||
| adc_sensor_mqtt() | ||
| { | ||
| for (uint8_t i = 0; i < UM_ADC_MQTT_PIN_MAX_NUMBER; i++) | ||
| { | ||
| #ifdef ESP8266 // static assert | ||
| adc_pin[0] = 17; // A0 | ||
| #else | ||
| adc_pin[i] = -1; | ||
| #endif | ||
| adc_value[i] = 0; | ||
| adc_Percentage[i] = 0; | ||
| adc_last_value[i] = 0; | ||
| device_class[i] = "voltage"; // default device class | ||
| unit_of_meas[i] = "V"; // default unit of measurement | ||
| } | ||
| // customize here your hass adc device class and unit of measurement | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add configuration options for device class settings in the constructor.
Provide common device class presets to make configuration easier for specific sensor types.
Add a configuration enum and extend the constructor to support common sensor types:
public:
+ enum SensorType {
+ VOLTAGE,
+ ILLUMINANCE,
+ TEMPERATURE,
+ HUMIDITY
+ };
+
+ void setSensorType(uint8_t index, SensorType type) {
+ if (index >= UM_ADC_MQTT_PIN_MAX_NUMBER) return;
+
+ switch(type) {
+ case ILLUMINANCE:
+ device_class[index] = "illuminance";
+ unit_of_meas[index] = "lx";
+ break;
+ case TEMPERATURE:
+ device_class[index] = "temperature";
+ unit_of_meas[index] = "°C";
+ break;
+ case HUMIDITY:
+ device_class[index] = "humidity";
+ unit_of_meas[index] = "%";
+ break;
+ case VOLTAGE:
+ default:
+ device_class[index] = "voltage";
+ unit_of_meas[index] = "V";
+ break;
+ }
+ }
adc_sensor_mqtt()
{
for (uint8_t i = 0; i < UM_ADC_MQTT_PIN_MAX_NUMBER; i++)
{
#ifdef ESP8266 // static assert
adc_pin[0] = 17; // A0
#else
adc_pin[i] = -1;
#endif
adc_value[i] = 0;
adc_Percentage[i] = 0;
adc_last_value[i] = 0;
- device_class[i] = "voltage"; // default device class
- unit_of_meas[i] = "V"; // default unit of measurement
+ setSensorType(i, ILLUMINANCE); // Default to illuminance for LDR sensors
}
- // customize here your hass adc device class and unit of measurement
}📝 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.
| public: | |
| adc_sensor_mqtt() | |
| { | |
| for (uint8_t i = 0; i < UM_ADC_MQTT_PIN_MAX_NUMBER; i++) | |
| { | |
| #ifdef ESP8266 // static assert | |
| adc_pin[0] = 17; // A0 | |
| #else | |
| adc_pin[i] = -1; | |
| #endif | |
| adc_value[i] = 0; | |
| adc_Percentage[i] = 0; | |
| adc_last_value[i] = 0; | |
| device_class[i] = "voltage"; // default device class | |
| unit_of_meas[i] = "V"; // default unit of measurement | |
| } | |
| // customize here your hass adc device class and unit of measurement | |
| } | |
| public: | |
| enum SensorType { | |
| VOLTAGE, | |
| ILLUMINANCE, | |
| TEMPERATURE, | |
| HUMIDITY | |
| }; | |
| void setSensorType(uint8_t index, SensorType type) { | |
| if (index >= UM_ADC_MQTT_PIN_MAX_NUMBER) return; | |
| switch(type) { | |
| case ILLUMINANCE: | |
| device_class[index] = "illuminance"; | |
| unit_of_meas[index] = "lx"; | |
| break; | |
| case TEMPERATURE: | |
| device_class[index] = "temperature"; | |
| unit_of_meas[index] = "°C"; | |
| break; | |
| case HUMIDITY: | |
| device_class[index] = "humidity"; | |
| unit_of_meas[index] = "%"; | |
| break; | |
| case VOLTAGE: | |
| default: | |
| device_class[index] = "voltage"; | |
| unit_of_meas[index] = "V"; | |
| break; | |
| } | |
| } | |
| adc_sensor_mqtt() | |
| { | |
| for (uint8_t i = 0; i < UM_ADC_MQTT_PIN_MAX_NUMBER; i++) | |
| { | |
| #ifdef ESP8266 // static assert | |
| adc_pin[0] = 17; // A0 | |
| #else | |
| adc_pin[i] = -1; | |
| #endif | |
| adc_value[i] = 0; | |
| adc_Percentage[i] = 0; | |
| adc_last_value[i] = 0; | |
| setSensorType(i, ILLUMINANCE); // Default to illuminance for LDR sensors | |
| } | |
| } |
…fig . improved loop & update
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp (3)
173-205: 🛠️ Refactor suggestionAdd a dropdown menu for sensor type selection in the configuration UI.
To improve user experience, add a dropdown menu for selecting common sensor types instead of requiring manual entry of device class and unit of measurement.
JsonObject top = root.createNestedObject(FPSTR(_name)); top["Enabled"] = adc_enabled; + // Add dropdown for sensor type selection + JsonArray sensorTypes = top.createNestedArray("SensorTypes"); + sensorTypes.add("Voltage"); + sensorTypes.add("Illuminance"); + sensorTypes.add("Temperature"); + sensorTypes.add("Humidity"); + for (uint8_t i = 0; i < UM_ADC_MQTT_PIN_MAX_NUMBER; i++) { // debug DEBUG_PRINT(F("configuring adc_sensor_mqtt: Pin ")); DEBUG_PRINT(i); #ifndef ESP8266 // esp8266 always use A0 no use of pin choice String adc_pin_name = F("AdcPin_"); adc_pin_name += String(i); top[adc_pin_name] = adc_pin[i]; #endif + String sensor_type_name = F("SensorType_"); + sensor_type_name += String(i); + // Add dropdown for each sensor + JsonObject dropDown = top.createNestedObject(sensor_type_name); + dropDown["type"] = "select"; + dropDown["value"] = getSensorTypeIndex(device_class[i]); + dropDown["options"] = sensorTypes; + // debug print new pin String device_class_name = F("DeviceClass_"); device_class_name += String(i); top[device_class_name] = device_class[i].c_str();Add a helper method to get the sensor type index:
private: uint8_t getSensorTypeIndex(const String& deviceClass) { if (deviceClass == "illuminance") return 1; if (deviceClass == "temperature") return 2; if (deviceClass == "humidity") return 3; return 0; // default to voltage }
142-147:⚠️ Potential issueFix incorrect ADC percentage calculation formulas.
The current formulas for calculating percentage values (both inverted and non-inverted) are unnecessarily complex and might produce unexpected results.
Apply this diff to simplify and fix the percentage calculations:
- adc_Percentage[i] = 100.0 - ((float)adc_value[i] * -1 + mapping) / mapping * 100.0f; + adc_Percentage[i] = 100.0 - ((float)adc_value[i] / mapping * 100.0f); - adc_Percentage[i] = ((float)adc_value[i] * -1 + mapping) / mapping * 100.0f; + adc_Percentage[i] = ((float)adc_value[i] / mapping * 100.0f);
51-67: 🛠️ Refactor suggestionAdd sensor type presets to improve usability.
Instead of requiring users to manually configure device_class and unit_of_measurement, adding preset configurations for common sensor types would improve the user experience.
public: + enum SensorType { + VOLTAGE, + ILLUMINANCE, + TEMPERATURE, + HUMIDITY + }; + + void setSensorType(uint8_t index, SensorType type) { + if (index >= UM_ADC_MQTT_PIN_MAX_NUMBER) return; + + switch(type) { + case ILLUMINANCE: + device_class[index] = "illuminance"; + unit_of_meas[index] = "lx"; + break; + case TEMPERATURE: + device_class[index] = "temperature"; + unit_of_meas[index] = "°C"; + break; + case HUMIDITY: + device_class[index] = "humidity"; + unit_of_meas[index] = "%"; + break; + case VOLTAGE: + default: + device_class[index] = "voltage"; + unit_of_meas[index] = "V"; + break; + } + } adc_sensor_mqtt() { for (uint8_t i = 0; i < UM_ADC_MQTT_PIN_MAX_NUMBER; i++) { #ifdef ESP8266 // static assert adc_pin[0] = 17; // A0 #else adc_pin[i] = -1; #endif adc_value[i] = 0; adc_Percentage[i] = 0; adc_last_value[i] = 0; - device_class[i] = "voltage"; // default device class - unit_of_meas[i] = "V"; // default unit of measurement + setSensorType(i, ILLUMINANCE); // Default to illuminance for LDR sensors } - // customize here your hass adc device class and unit of measurement }
🧹 Nitpick comments (4)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp (4)
121-126: Improve the force update logic for better clarity.The current force update logic is complex and could be made more readable with clearer variable names and comments.
- bool force_update = (millis() - lastTime) > (update_interval * 100); // to keep the mqtt alive if the light is not changing - bool regular_update = (millis() - lastTime) > update_interval; - if (regular_update || force_update ) // force update if the time is up or if the light is not changing to retain mqtt hass activity in case + // Update on regular interval or periodically to keep MQTT connection alive + const unsigned long currentTime = millis(); + const unsigned long timeSinceLastUpdate = currentTime - lastTime; + const bool isRegularUpdateDue = timeSinceLastUpdate > update_interval; + const bool isLongTermUpdateDue = timeSinceLastUpdate > (update_interval * 100); + + if (isRegularUpdateDue || isLongTermUpdateDue) { - bool force_first_update = ((published_initial_value == false) && WLED_MQTT_CONNECTED); - lastTime = millis(); // reset lastTime to current time + // Force first update if MQTT is connected but no values were published yet + const bool isFirstUpdateNeeded = !published_initial_value && WLED_MQTT_CONNECTED; + lastTime = currentTime; // reset lastTime to current time
365-412: Add error handling for MQTT operations and optimize JSON memory usage.The
publishDiscoverymethod could benefit from additional error handling and more efficient memory management.void publishDiscovery(uint8_t pin_number) { - StaticJsonDocument<700> doc; + // Calculate buffer size based on the maximum possible content + const size_t capacity = JSON_OBJECT_SIZE(11) + JSON_OBJECT_SIZE(7) + JSON_ARRAY_SIZE(2) + 256; + StaticJsonDocument<capacity> doc; String uid = escapedMac.c_str(); uid += F("_adc_"); uid += String(pin_number); @@ -410,7 +410,15 @@ discovery_topic += F("/config"); char json_str[700]; size_t payload_size = serializeJson(doc, json_str); - hassDiscoverySent = mqtt->publish(discovery_topic.c_str(), 1, true, json_str, payload_size) > 0; // publish discovery message + + // Publish discovery message and handle errors + bool publishSuccess = mqtt->publish(discovery_topic.c_str(), 1, true, json_str, payload_size) > 0; + if (publishSuccess) { + hassDiscoverySent = true; + DEBUG_PRINTLN(F("Discovery message sent successfully")); + } else { + DEBUG_PRINTLN(F("Failed to send discovery message!")); + } }
340-363: Add retry mechanism for MQTT publishing.The
publishMqttmethod could be enhanced with a retry mechanism for more reliable communication.void publishMqtt(int index, float state) { if (WLED_MQTT_CONNECTED) { DEBUG_PRINTLN(F("Publishing MQTT message...")); + const uint8_t maxRetries = 3; + uint8_t retryCount = 0; + bool publishSuccess = false; + if (!hassDiscoverySent && HomeAssistantDiscovery) { // publish all discovery message if not already done @@ -354,7 +362,19 @@ String mqtt_stat_topic = mqttDeviceTopic; mqtt_stat_topic += MQTT_TOPIC; mqtt_stat_topic += String(index); - mqtt->publish(mqtt_stat_topic.c_str(), 0, true, String(state, 2).c_str()); + + // Attempt publishing with retries + String stateStr = String(state, 2); + while (!publishSuccess && retryCount < maxRetries) { + publishSuccess = mqtt->publish(mqtt_stat_topic.c_str(), 0, true, stateStr.c_str()); + if (!publishSuccess) { + retryCount++; + DEBUG_PRINT(F("MQTT publish failed, retry ")); + DEBUG_PRINT(retryCount); + DEBUG_PRINTLN(F(" of 3")); + delay(100); // Short delay before retry + } + } published_initial_value = true; DEBUG_PRINT(F("MQTT message sent: ")); DEBUG_PRINT(mqtt_stat_topic.c_str());
149-166: Improve change threshold handling to avoid repetitive calculations.The current implementation recalculates whether there's a meaningful change without considering the percentage value, which might lead to unnecessary MQTT updates.
- if (abs(adc_value[i] - adc_last_value[i]) > change_threshold || force_update || force_first_update) + // Calculate the percentage change relative to the full range + float percentageChange = abs(adc_value[i] - adc_last_value[i]) / mapping * 100.0f; + + if (percentageChange > change_threshold || force_update || force_first_update) { adc_last_value[i] = adc_value[i]; char buf[64]; - sprintf_P(buf, PSTR("adc %d: %d (%d)"), i, static_cast<uint8_t>(adc_Percentage[i]), adc_value[i]); + sprintf_P(buf, PSTR("adc %d: %.1f%% (%d) - change: %.1f%%"), + i, adc_Percentage[i], adc_value[i], percentageChange); DEBUG_PRINTLN(buf); if (WLED_MQTT_CONNECTED)
it is ready now, i tested it well on multiple targets :) . let me know you opinion |
|
generally looks really good, I have no means to test any of it though. |
i tested it on 5 targets normal ESP32 and C3/S2/S3 and esp8266. no monkey business found since couple of days in my setup. |
DedeHai
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
usermods/adc_sensor_mqtt/README.md (5)
1-3: Fix markdown heading style and improve initial description clarity.The top-level heading has an extra trailing
##, and the description line contains grammar and punctuation issues. Consider:
- Removing the stray
##or using a matching closing#(e.g.,# ADC sensor with MQTT & HASS support #).- Rewriting the description for clarity, capitalizing acronyms (
ADC,HASS) and fixing sentence structure.Proposed diff:
-# ADC sensor with MQTT & HASS support ## -This usermod will obtain readings from adc pin. This is useful for ldr for example for exterior lighting situations where you want the lights to only be on when it is dark out. but this mod is designed for more hass application. it will publish auto discovery message to hass mqtt and attach the sensor to the wled integration automatically +# ADC sensor with MQTT & HASS support # + +This usermod reads analog values from an ADC pin (e.g., using an LDR for exterior lighting). It publishes Home Assistant MQTT auto-discovery messages, automatically attaching the sensor to the WLED integration.🧰 Tools
🪛 LanguageTool
[uncategorized] ~2-~2: A comma might be missing here.
Context: ...adings from adc pin. This is useful for ldr for example for exterior lighting situa...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~2-~2: A comma might be missing here.
Context: ...rom adc pin. This is useful for ldr for example for exterior lighting situations where ...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
4-12: Specify fenced code block language and use inline code formatting.The installation example fence is missing a language identifier, and the usermod name should be formatted as inline code. Update the snippet as follows:
-``` +```ini [env:adc_sensor_mqtt_esp32dev] extends = env:esp32dev custom_usermods = ${env:esp32dev.custom_usermods} - adc_sensor_mqtt # Enable ADC sensor with MQTT + `adc_sensor_mqtt` # Enable ADC sensor with MQTT -``` +```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
4-4: Heading style
Expected: atx_closed; Actual: atx(MD003, heading-style)
7-7: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
14-16: Surround tables with blank lines and close headings.Markdown tables should be preceded and followed by blank lines, and headings should have matching closing hashes:
-# Usermod Settings -Setting | Description | Default --- | --- | --- +# Usermod Settings # + +Setting | Description | Default +--- | --- | --- +🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
14-14: Heading style
Expected: atx_closed; Actual: atx(MD003, heading-style)
15-15: Tables should be surrounded by blank lines
null(MD058, blanks-around-tables)
17-26: Standardize table entries and improve wording.Ensure each setting:
- Uses inline code formatting for the setting name (e.g.,
`Pin`).- Capitalizes acronyms (
ESP8266,ESP32,HASS).- Has clear and consistent descriptions.
Example adjustments:
-Enabled | Enable/Disable the LDR functionality. | Disabled +`Enabled` | Enable or disable the analog sensor functionality. | false -Pin | The analog capable pin your LDR is connected to. | A0 is forced in esp8266 and its not optional +`Pin` | The analog-capable pin the sensor is connected to (ESP8266 only supports A0). | platform default -AdcUpdateInterval | update interval to read the analog pin ( in ms ) | 3000 +`AdcUpdateInterval` | Interval in milliseconds between readings. | 3000 -Inverted | invert readings based on your own hardware setup ( invert mapping analog to digital value to 0V = 100% or 3.3V = 100% ) | false +`Inverted` | Invert the mapping (0 V = 100 % or 3.3 V = 100 %). | false -ChangeThreshold | the minimum limit to detect change and publish the value ; checks the raw value ( int ; 5 ) . | 1 +`ChangeThreshold` | Minimum delta in raw value to trigger an MQTT update. | 1 -HASS | Enable home assistant mqtt discovery message with wled integration | true +`HASS` | Enable Home Assistant MQTT auto-discovery. | true -Raw | publish the ADC value as raw measurement (0-4096 for ESP32 / 0-1024 for ESP8266) | false +`Raw` | Publish the raw ADC value (0 – 4095 on ESP32, 0 – 1023 on ESP8266). | false -DeviceClass | HASS sensor discovery device class | voltage +`DeviceClass` | Home Assistant sensor device class. | voltage -UnitOfMeas | HASS sensor discovery device unit of measurements | V +`UnitOfMeas` | Home Assistant sensor unit of measurement. | V
27-28: Correct author link formatting and close heading.There's an unintended space breaking the link syntax, and the heading should close with matching hashes:
-## Author -[@rommo911] (https://github.com/rommo911) +## Author ## +[@rommo911](https://github.com/rommo911)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
27-27: Heading style
Expected: atx_closed; Actual: atx(MD003, heading-style)
28-28: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
usermods/adc_sensor_mqtt/README.md(1 hunks)usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp
🧰 Additional context used
🪛 LanguageTool
usermods/adc_sensor_mqtt/README.md
[uncategorized] ~2-~2: A comma might be missing here.
Context: ...adings from adc pin. This is useful for ldr for example for exterior lighting situa...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~2-~2: A comma might be missing here.
Context: ...rom adc pin. This is useful for ldr for example for exterior lighting situations where ...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
🪛 markdownlint-cli2 (0.17.2)
usermods/adc_sensor_mqtt/README.md
4-4: Heading style
Expected: atx_closed; Actual: atx
(MD003, heading-style)
7-7: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
14-14: Heading style
Expected: atx_closed; Actual: atx
(MD003, heading-style)
15-15: Tables should be surrounded by blank lines
null
(MD058, blanks-around-tables)
27-27: Heading style
Expected: atx_closed; Actual: atx
(MD003, heading-style)
28-28: Bare URL used
null
(MD034, no-bare-urls)
|
it looks pretty decent now
in HASS and added simple section to customize number of available sensor class/unit and their values properties |
|
is the change threshold in ADC bits? would that be better to do in % of full scale or something more accessible to the "unknowing" user? |
Yeah i though about it but it can be set to percentage but in this case i want it to be float value, example , getting change threshold to 0.5% or 2.5% , or in this case the default mapped value. |
Refines ADC value mapping and change detection logic.
yeah i added logic to detect change either in raw value if activated or in mapped ( % ) value if raw value is disabled . |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp (8)
38-39: Home Assistant Discovery enabled by default - consider potential impact.The usermod enables HomeAssistantDiscovery by default, which will automatically send MQTT discovery messages when a device connects. While convenient for Home Assistant users, this could cause unnecessary network traffic for users who don't use Home Assistant.
Consider making this opt-in rather than opt-out for new installations:
- bool HomeAssistantDiscovery = true; // is HA discovery turned on by default + bool HomeAssistantDiscovery = false; // is HA discovery turned on by default
89-106: Improve error handling for pin allocation.The current implementation doesn't clearly distinguish between different types of pin allocation failures or provide enough debugging information.
Improve the error handling with more specific messages:
if ((adc_pin[i] != -1) && adc_enabled) { #ifdef ESP8266 if (adc_pin[i] == 17) // only one Pin supported : A0 for esp8266 // #else - if ((_valid_adc_pin(adc_pin[i])) && PinManager::allocatePin(adc_pin[i], false, PinOwner::UM_ADC_MQTT)) // ESP32 only pins 32-39 are available for ADC + if (!_valid_adc_pin(adc_pin[i])) { + DEBUG_PRINT(F("adc_sensor_mqtt: Pin ")); + DEBUG_PRINT(adc_pin[i]); + DEBUG_PRINTLN(F(" is not in valid ADC range!")); + adc_pin[i] = -1; // invalid pin -> disable this pin + } + else if (!PinManager::allocatePin(adc_pin[i], false, PinOwner::UM_ADC_MQTT)) { + DEBUG_PRINT(F("adc_sensor_mqtt: Pin ")); + DEBUG_PRINT(adc_pin[i]); + DEBUG_PRINTLN(F(" allocation failed, pin may be in use!")); + adc_pin[i] = -1; // allocation failed -> disable this pin + } + else // valid pin and allocation successful #endif { pinMode(adc_pin[i], INPUT); // alloc success -> configure pin for input valid_pins++; } else { - DEBUG_PRINTLN(F("adc_sensor_mqtt: Pin allocation failed!")); adc_pin[i] = -1; // invalid pin -> disable this pin }
136-137: Improve logic for force update timing.The current implementation uses
update_interval * 100for force updates, which might be excessive for longer update intervals. For example, with a 2000ms interval, this would be 200,000ms (over 3 minutes).Consider using a more reasonable interval for force updates:
- bool force_update = (millis() - lastTime) > (update_interval * 100); // to keep the mqtt alive if the light is not changing + bool force_update = (millis() - lastTime) > min(update_interval * 10, 60000); // Force update at least every minute
154-158: Improve debug print format and efficiency.The current debug message formatting could be improved, and there's a type conversion that might lose precision.
Refine the debug output:
- adc_last_mapped_value[i] = adc_mapped_value[i]; - char buf[64]; - sprintf_P(buf, PSTR("adc %d: %d (%d)"), i, static_cast<uint8_t>(adc_mapped_value[i]), adc_value[i]); - DEBUG_PRINTLN(buf); + adc_last_mapped_value[i] = this->publishRawValue ? adc_value[i] : adc_mapped_value[i]; + DEBUG_PRINTF("adc %d: %.2f (%d)\n", i, adc_mapped_value[i], adc_value[i]);
378-378: MQTT payload format could cause precision issues.The current code publishes MQTT messages with 2 decimal places regardless of the data type or range, which might cause issues with some sensors requiring higher precision.
Consider adapting the precision based on the device class:
- mqtt->publish(mqtt_stat_topic.c_str(), 0, true, String(state, 2).c_str()); + // Adjust precision based on device class + int precision = 2; // Default precision + if (device_class[index] == "temperature") precision = 1; + else if (device_class[index] == "illuminance") precision = 0; + mqtt->publish(mqtt_stat_topic.c_str(), 0, true, String(state, precision).c_str());
385-385: Potential memory issue with MQTT topics.The code creates multiple String objects for MQTT topics, which could lead to memory fragmentation over time, especially on ESP8266 with limited RAM.
Consider using static buffers instead of String objects:
void publishMqtt(int index, float state) { if (WLED_MQTT_CONNECTED) { DEBUG_PRINTLN(F("Publishing MQTT message...")); if (!hassDiscoverySent && HomeAssistantDiscovery) { // publish all discovery message if not already done for (uint8_t i = 0; i < UM_ADC_MQTT_PIN_MAX_NUMBER; i++) { publishDiscovery(i); } } - String mqtt_stat_topic = mqttDeviceTopic; - mqtt_stat_topic += MQTT_TOPIC; - mqtt_stat_topic += String(index); + char mqtt_stat_topic[128]; + snprintf_P(mqtt_stat_topic, sizeof(mqtt_stat_topic), PSTR("%s%s%d"), + mqttDeviceTopic.c_str(), MQTT_TOPIC, index); + + char value_str[16]; + dtostrf(state, 0, 2, value_str); - mqtt->publish(mqtt_stat_topic.c_str(), 0, true, String(state, 2).c_str()); + mqtt->publish(mqtt_stat_topic, 0, true, value_str);
400-405: Consider additional Home Assistant metadata for better integration.The Home Assistant discovery message could benefit from additional metadata to improve integration.
Add more metadata to the Home Assistant discovery message:
doc[F("stat_t")] = mqtt_stat_topic; doc[F("dev_cla")] = device_class[pin_number].c_str(); if (publishRawValue == false) { doc[F("unit_of_meas")] = unit_of_meas[pin_number].c_str(); } + // Add state class for better graphing in Home Assistant + doc[F("stat_cla")] = F("measurement"); + // Add entity category + doc[F("ent_cat")] = F("diagnostic");
447-448: Add battery as a device class option as discussed in PR feedback.In the PR feedback, you discussed adding battery as one of the device class options, but it's not included in the final implementation.
Add battery as a device class option:
- const String adc_sensor_mqtt::device_classes[NUMBER_OF_DEFAULT_SENSOR_CLASSES] PROGMEM = {"illuminance", "current", "power", "temperature", "voltage"}; // default device class - const String adc_sensor_mqtt::device_unit_of_measurement[NUMBER_OF_DEFAULT_SENSOR_CLASSES] PROGMEM = {"lx", "A", "W", "°C", "V"}; // default unit of measurement + // Update NUMBER_OF_DEFAULT_SENSOR_CLASSES to 6 in the class definition + const String adc_sensor_mqtt::device_classes[NUMBER_OF_DEFAULT_SENSOR_CLASSES] PROGMEM = {"illuminance", "current", "power", "temperature", "voltage", "battery"}; // default device class + const String adc_sensor_mqtt::device_unit_of_measurement[NUMBER_OF_DEFAULT_SENSOR_CLASSES] PROGMEM = {"lx", "A", "W", "°C", "V", "%"}; // default unit of measurement
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp(1 hunks)
🔇 Additional comments (2)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp (2)
64-64: LGTM! The mapFloat function implementation is correct.The function properly handles mapping values from one range to another using floating-point arithmetic.
267-305: Great work implementing the dropdown selections for device class and unit of measurement!The UI configuration code provides an excellent user experience with dropdown options for device classes and units of measurement. This implementation addresses the previous feedback about providing user-friendly selection options.
| adc_mapped_value[i] = read_adc_mapping(adc_value[i]); // read adc value and map it to percentage | ||
| bool changed = this->publishRawValue ? (adc_value[i] != adc_last_mapped_value[i]) : (adc_mapped_value[i] != adc_last_mapped_value[i]); |
There was a problem hiding this comment.
Fix comparison logic in change detection.
The current implementation compares the current mapped value with the last mapped value, but stores the mapped value in adc_last_mapped_value even when using raw values.
Update the comparison logic to properly track changes based on publishing mode:
- adc_mapped_value[i] = read_adc_mapping(adc_value[i]); // read adc value and map it to percentage
- bool changed = this->publishRawValue ? (adc_value[i] != adc_last_mapped_value[i]) : (adc_mapped_value[i] != adc_last_mapped_value[i]);
+ adc_mapped_value[i] = read_adc_mapping(adc_value[i]); // read adc value and map it to percentage
+ bool changed;
+ if (this->publishRawValue) {
+ changed = abs(adc_value[i] - adc_last_mapped_value[i]) > change_threshold;
+ } else {
+ changed = abs(adc_mapped_value[i] - adc_last_mapped_value[i]) > change_threshold;
+ }Committable suggestion skipped: line range outside the PR's diff.
| { | ||
| #ifdef ESP32 | ||
| const uint16_t mapping = 4096; // ESP32 ADC resolution is 12 bit, so 4096 values | ||
| #else | ||
| const uint16_t mapping = 1024; // ESP8266 ADC resolution is 10 bit, so 1024 values | ||
| #endif | ||
| if (adc_sensor_mqtt::inverted) // default mapping value to 0-100 -- replace here your own mapping for your own unit of measurement | ||
| { | ||
| return mapFloat(rawValue, 0, mapping, 100, 0); // map raw value to percentage | ||
| } | ||
| else | ||
| { | ||
| return mapFloat(rawValue, 0, mapping, 0, 100); // map raw value to percentage | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance ADC mapping flexibility.
The current mapping function only supports percentage values. To be more generic, it would be useful to support custom scaling based on the selected device class.
Enhance the ADC mapping function to support different device classes:
float adc_sensor_mqtt::read_adc_mapping(uint16_t rawValue)
{
#ifdef ESP32
const uint16_t mapping = 4096; // ESP32 ADC resolution is 12 bit, so 4096 values
#else
const uint16_t mapping = 1024; // ESP8266 ADC resolution is 10 bit, so 1024 values
#endif
+
+ // Default percentage mapping (0-100%)
if (adc_sensor_mqtt::inverted) // default mapping value to 0-100 -- replace here your own mapping for your own unit of measurement
{
return mapFloat(rawValue, 0, mapping, 100, 0); // map raw value to percentage
}
else
{
return mapFloat(rawValue, 0, mapping, 0, 100); // map raw value to percentage
}
+
+ // Add custom mappings for specific device classes as needed
+ // Example for a voltage sensor (assuming 0V-3.3V range):
+ // if (device_class == "voltage") {
+ // return mapFloat(rawValue, 0, mapping, 0, 3.3);
+ // }
}| configComplete &= getJsonValue(top[str][F("Pin")], adc_pin[i]); | ||
| const bool valid_adc_pin = _valid_adc_pin(adc_pin[i]); // only ADC1 available on ESP32 with wifi | ||
| if (!valid_adc_pin || !PinManager::isPinOk(adc_pin[i], false)) // // ESP32 only pins 32-39 are available for ADC | ||
| { | ||
| DEBUG_PRINT(F("adc_sensor_mqtt: Pin ")); | ||
| DEBUG_PRINT(i); | ||
| DEBUG_PRINTLN(F(" is not valid!")); | ||
| configComplete = false; // pin not valid -> disable usermod | ||
| adc_pin[i] = -1; // invalid pin -> disable usermod | ||
| } | ||
| else |
There was a problem hiding this comment.
Fix logical error in pin validation.
The code has a logical error where configComplete is incorrectly set to false and the usermod is disabled when a pin is invalid, but the loop continues and might process valid pins later.
Correct the validation logic:
configComplete &= getJsonValue(top[str][F("Pin")], adc_pin[i]);
const bool valid_adc_pin = _valid_adc_pin(adc_pin[i]); // only ADC1 available on ESP32 with wifi
if (!valid_adc_pin || !PinManager::isPinOk(adc_pin[i], false)) // // ESP32 only pins 32-39 are available for ADC
{
DEBUG_PRINT(F("adc_sensor_mqtt: Pin "));
DEBUG_PRINT(i);
DEBUG_PRINTLN(F(" is not valid!"));
- configComplete = false; // pin not valid -> disable usermod
adc_pin[i] = -1; // invalid pin -> disable usermod
}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp (2)
148-149:⚠️ Potential issueFix comparison logic in change detection.
The current implementation compares the current mapped value with the last mapped value, but stores the mapped value in
adc_last_mapped_valueeven when using raw values.adc_mapped_value[i] = read_adc_mapping(adc_value[i]); // read adc value and map it to percentage -bool changed = this->publishRawValue ? (adc_value[i] != adc_last_mapped_value[i]) : (adc_mapped_value[i] != adc_last_mapped_value[i]); +bool changed; +if (this->publishRawValue) { + changed = abs(adc_value[i] - adc_last_mapped_value[i]) > change_threshold; +} else { + changed = abs(adc_mapped_value[i] - adc_last_mapped_value[i]) > change_threshold; +}
227-237:⚠️ Potential issueFix logical error in pin validation.
The code has a logical error where
configCompleteis incorrectly set to false and the usermod is disabled when a pin is invalid, but the loop continues and might process valid pins later.configComplete &= getJsonValue(top[str][F("Pin")], adc_pin[i]); const bool valid_adc_pin = _valid_adc_pin(adc_pin[i]); // only ADC1 available on ESP32 with wifi if (!valid_adc_pin || !PinManager::isPinOk(adc_pin[i], false)) // // ESP32 only pins 32-39 are available for ADC { DEBUG_PRINT(F("adc_sensor_mqtt: Pin ")); DEBUG_PRINT(i); DEBUG_PRINTLN(F(" is not valid!")); - configComplete = false; // pin not valid -> disable usermod adc_pin[i] = -1; // invalid pin -> disable usermod }
🧹 Nitpick comments (3)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp (3)
33-55: Member variables are well organized but consider naming convention consistency.The class variables are well-structured, but there's inconsistency in naming conventions. WLED typically uses camelCase for variable names (like
lastTime), but many variables use snake_case (likeadc_pin,adc_value). Consider standardizing on camelCase for better consistency with the rest of the WLED codebase.
58-61: Add parameter validation to mapFloat function.The mapFloat function could encounter a division by zero error if in_max equals in_min.
static inline float mapFloat(int x, int in_min, int in_max, float out_min, float out_max) { + if (in_max == in_min) return out_min; // Prevent division by zero return ((float)(x - in_min) * (out_max - out_min)) / (float)(in_max - in_min) + out_min; }
453-468: Enhance ADC mapping flexibility.The current mapping function only supports percentage values. To be more generic, it would be useful to support custom scaling based on the selected device class.
float adc_sensor_mqtt::read_adc_mapping(uint16_t rawValue) { #ifdef ESP32 const uint16_t mapping = 4096; // ESP32 ADC resolution is 12 bit, so 4096 values #else const uint16_t mapping = 1024; // ESP8266 ADC resolution is 10 bit, so 1024 values #endif + + // Default percentage mapping (0-100%) if (adc_sensor_mqtt::inverted) // default mapping value to 0-100 -- replace here your own mapping for your own unit of measurement { return mapFloat(rawValue, 0, mapping, 100, 0); // map raw value to percentage } else { return mapFloat(rawValue, 0, mapping, 0, 100); // map raw value to percentage } + + // Add custom mappings for specific device classes as needed + // Example for a voltage sensor (assuming 0V-3.3V range): + // if (device_class == "voltage") { + // return mapFloat(rawValue, 0, mapping, 0, 3.3); + // } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp(1 hunks)
🔇 Additional comments (4)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp (4)
1-29: Platform-specific configuration looks good.The preprocessor directives handle different ESP variants correctly, defining appropriate pin ranges for each target platform. The static assert for ESP8266 ensures that only one ADC pin is defined, which matches hardware capabilities.
266-302: Good UI implementation with dropdown selections.The dropdown implementation for device class and unit selection enhances user experience, making it easier to configure the sensor. This is an excellent improvement over text fields.
359-382: MQTT publishing implementation looks good.The method to publish sensor values to MQTT handles all necessary aspects - checking connection status, handling discovery messages, and formatting the payload correctly.
384-434: Comprehensive Home Assistant discovery implementation.The discovery message includes all necessary information for Home Assistant to properly integrate the sensor, including device metadata, availability topics, and sensor-specific configuration. Good attention to detail with hardware version detection and connection information.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp (1)
225-235:⚠️ Potential issueFix logical error in pin validation
The pin validation logic incorrectly disables the entire usermod if any pin is invalid by setting
configComplete = false. This prevents the usermod from working even when other pins are valid.configComplete &= getJsonValue(top[str][F("Pin")], adc_pin[i]); const bool valid_adc_pin = _valid_adc_pin(adc_pin[i]); // only ADC1 available on ESP32 with wifi if (!valid_adc_pin || !PinManager::isPinOk(adc_pin[i], false)) // // ESP32 only pins 32-39 are available for ADC { DEBUG_PRINT(F("adc_sensor_mqtt: Pin ")); DEBUG_PRINT(i); DEBUG_PRINTLN(F(" is not valid!")); - configComplete = false; // pin not valid -> disable usermod adc_pin[i] = -1; // invalid pin -> disable usermod }
🧹 Nitpick comments (2)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp (2)
63-79: Remove outdated comment about customizing HASS settingsThe comment on line 78 about customizing HASS device class and unit of measurement is now outdated since these settings are handled through the configuration UI.
} - // customize here your hass adc device class and unit of measurement }
451-466: Consider enhancing the ADC mapping to support different sensor typesThe current mapping function only supports percentage conversion. For greater flexibility, consider extending it to support different mappings based on the selected device class.
float adc_sensor_mqtt::read_adc_mapping(uint16_t rawValue) { #ifdef ESP32 const uint16_t mapping = 4096; // ESP32 ADC resolution is 12 bit, so 4096 values #else const uint16_t mapping = 1024; // ESP8266 ADC resolution is 10 bit, so 1024 values #endif + + // This function could be extended to support different mappings based on the device class + // For example, for voltage sensors, map to 0-3.3V depending on the ESP platform + if (adc_sensor_mqtt::inverted) // default mapping value to 0-100 -- replace here your own mapping for your own unit of measurement { return mapFloat(rawValue, 0, mapping, 100, 0); // map raw value to percentage } else { return mapFloat(rawValue, 0, mapping, 0, 100); // map raw value to percentage } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp(1 hunks)
🔇 Additional comments (3)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp (3)
16-29: Good platform-specific handling for ESP32 variantsThe code properly defines valid ADC pin ranges for different ESP32 variants using preprocessor conditionals. This ensures the usermod works correctly across the ESP32 family.
264-300: Good UI implementation with dropdowns for device class and unitThe UI configuration is well-implemented with dropdown menus for device class and unit of measurement, which greatly improves user experience as requested in the PR feedback.
382-432: Good Home Assistant discovery implementationThe Home Assistant discovery implementation is comprehensive and includes all necessary fields for proper integration, including unique ID, name, state topic, device class, unit of measurement, and device metadata.
| // DEBUG_PRINTLN(F("adc_sensor_mqtt:loop Pin not valid!")); | ||
| continue; // skip if pin is not valid | ||
| } | ||
| bool changed = this->publishRawValue ? (abs(adc_value[i] - analogRead(adc_pin[i])) > change_threshold) : (abs(adc_mapped_value[i] - read_adc_mapping(analogRead(adc_pin[i]))) > change_threshold); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize ADC reading to avoid redundant analogRead() calls
The current change detection logic reads the analog pin twice - once for comparison and again for storage. This is inefficient and may lead to inconsistent values if the analog reading changes between calls.
- bool changed = this->publishRawValue ? (abs(adc_value[i] - analogRead(adc_pin[i])) > change_threshold) : (abs(adc_mapped_value[i] - read_adc_mapping(analogRead(adc_pin[i]))) > change_threshold);
+ uint16_t current_reading = analogRead(adc_pin[i]);
+ float current_mapped = read_adc_mapping(current_reading);
+ bool changed = this->publishRawValue ?
+ (abs(adc_value[i] - current_reading) > change_threshold) :
+ (abs(adc_mapped_value[i] - current_mapped) > change_threshold);📝 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.
| bool changed = this->publishRawValue ? (abs(adc_value[i] - analogRead(adc_pin[i])) > change_threshold) : (abs(adc_mapped_value[i] - read_adc_mapping(analogRead(adc_pin[i]))) > change_threshold); | |
| // Cache the ADC reading and its mapped value to avoid double reads | |
| uint16_t current_reading = analogRead(adc_pin[i]); | |
| float current_mapped = read_adc_mapping(current_reading); | |
| bool changed = this->publishRawValue | |
| ? (abs(adc_value[i] - current_reading) > change_threshold) | |
| : (abs(adc_mapped_value[i] - current_mapped) > change_threshold); |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp (4)
452-466: Enhance ADC mapping flexibility.The current mapping function only supports percentage values. To be more generic, it would be useful to support custom scaling based on the selected device class.
float adc_sensor_mqtt::read_adc_mapping(uint16_t rawValue) { #ifdef ESP32 const uint16_t mapping = 4096; // ESP32 ADC resolution is 12 bit, so 4096 values #else const uint16_t mapping = 1024; // ESP8266 ADC resolution is 10 bit, so 1024 values #endif + + // Default percentage mapping (0-100%) if (adc_sensor_mqtt::inverted) // default mapping value to 0-100 -- replace here your own mapping for your own unit of measurement { return mapFloat(rawValue, 0, mapping, 100, 0); // map raw value to percentage } else { return mapFloat(rawValue, 0, mapping, 0, 100); // map raw value to percentage } + + // Add custom mappings for specific device classes as needed + // Example for a voltage sensor (assuming 0V-3.3V range): + // if (device_class == "voltage") { + // return mapFloat(rawValue, 0, mapping, 0, 3.3); + // } }
78-78: Please add more device class options to the dropdown.As discussed in previous reviews, adding a selection of the most relevant device classes for WLED applications would improve usability.
Based on the previous discussion between DedeHai and rommo911, please verify that you've implemented the dropdown with the agreed-upon device classes:
- battery (%)
- illuminance (lx)
- temperature (°C)
- voltage (V)
- current (mA)
- None (N/A)
#!/bin/bash # Check if the device classes in the code match the agreed list grep -A5 "const char \\*adc_sensor_mqtt::device_classes" usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp
146-146: 🛠️ Refactor suggestionOptimize ADC reading to avoid redundant analogRead() calls.
The current change detection logic reads the analog pin twice - once for comparison and again for storage. This is inefficient and may lead to inconsistent values if the analog reading changes between calls.
- bool changed = this->publishRawValue ? (abs(adc_value[i] - analogRead(adc_pin[i])) > change_threshold) : (abs(adc_mapped_value[i] - read_adc_mapping(analogRead(adc_pin[i]))) > change_threshold); + uint16_t current_reading = analogRead(adc_pin[i]); + float current_mapped = read_adc_mapping(current_reading); + bool changed = this->publishRawValue ? + (abs(adc_value[i] - current_reading) > change_threshold) : + (abs(adc_mapped_value[i] - current_mapped) > change_threshold);
225-235:⚠️ Potential issueFix logical error in pin validation.
The code has a logical error where
configCompleteis incorrectly set to false and the usermod is disabled when a pin is invalid, but the loop continues and might process valid pins later.configComplete &= getJsonValue(top[str][F("Pin")], adc_pin[i]); const bool valid_adc_pin = _valid_adc_pin(adc_pin[i]); // only ADC1 available on ESP32 with wifi if (!valid_adc_pin || !PinManager::isPinOk(adc_pin[i], false)) // // ESP32 only pins 32-39 are available for ADC { DEBUG_PRINT(F("adc_sensor_mqtt: Pin ")); DEBUG_PRINT(i); DEBUG_PRINTLN(F(" is not valid!")); - configComplete = false; // pin not valid -> disable usermod adc_pin[i] = -1; // invalid pin -> disable usermod }
🧹 Nitpick comments (2)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp (2)
156-164: Simplify redundant MQTT publishing logic.The conditional statement is unnecessary since both branches call the same function with the same parameters.
- if (publishRawValue) - { - publishMqtt(i); - } - else - { - publishMqtt(i); - } + publishMqtt(i);
145-151: Add validation for extreme ADC values.Consider adding validation for the ADC readings to handle potential outliers or invalid readings.
uint16_t current_reading = analogRead(adc_pin[i]); + // Filter out invalid or extreme readings + #ifdef ESP32 + if (current_reading > 4095) current_reading = 4095; // ESP32 max ADC value + #else + if (current_reading > 1023) current_reading = 1023; // ESP8266 max ADC value + #endif adc_mapped_value[i] = read_adc_mapping(current_reading);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp (3)
wled00/pin_manager.h (5)
allocatePin(89-89)allocatePin(97-97)isPinOk(104-104)deallocatePin(83-83)deallocatePin(99-99)usermods/PIR_sensor_switch/PIR_sensor_switch.cpp (4)
publishMqtt(274-294)publishMqtt(274-274)setup(370-390)setup(370-370)usermods/SN_Photoresistor/SN_Photoresistor.cpp (1)
REGISTER_USERMOD(214-214)
🔇 Additional comments (5)
usermods/adc_sensor_mqtt/adc_sensor_mqtt.cpp (5)
16-29: Excellent platform-specific pin validation approach.The code correctly handles different pin configurations for various ESP32 variants (ESP32, ESP32-S2/S3, ESP32-C3), ensuring only ADC1 pins are used since ADC2 can't be used with WiFi active. This is a robust approach to cross-platform compatibility.
34-54: Well-organized member variables with clear initialization.The class member variables are logically organized with good naming conventions and sensible default values. The use of static const variables for device classes and units of measurement provides flexibility while maintaining memory efficiency.
272-300: Great configuration UI with dropdown menus for sensor types.The implementation of dropdown menus for device classes and units of measurement enhances usability, making it easy for users to configure the sensor without needing to look up valid Home Assistant device classes.
382-432: Comprehensive Home Assistant MQTT discovery implementation.The MQTT discovery implementation is thorough, providing all necessary information for Home Assistant to automatically discover and configure the sensor, including unique IDs, device metadata, and availability tracking.
468-470: Good implementation of usermod registration.The code correctly registers the usermod instance with the WLED framework using the standard REGISTER_USERMOD macro.
added new usermod for ldr with hass mqtt integration :
features : it will publish auto discovery message to hass mqtt and attach the light sensor to the wled integration automatically with good options
pir switch usermod : added support for discovery message to integrate mqtt entity into wled integration ( check photo )
othert changes : fixed signed/unsigned comparasion compiler warning , added to gitignore local buildcache folder ( when desired )
Summary by CodeRabbit
New Features
Documentation
Chores