Added ability to test if Usermod is enabled from UsermodManager

[rhkean]

• added ability to check if a usermod is enabled from UsermodManager
• added isEnabled() to Usermod base class
• standardized mod_id parameter in UsermodManager (some were uint8_t, some were uint16_t)

Summary by CodeRabbit

• New Features

  • Added the ability to check if a user module is enabled.

• Improvements

  • Expanded support for a wider range of user module IDs.

[coderabbitai]

Walkthrough

The changes introduce enhancements to the usermod management system. A new virtual method, isEnabled(), is added to the Usermod class, providing a mechanism to determine if a usermod is active, with a default return value of true. The parameter type for mod_id in the UsermodManager::getUMData function is updated from uint8_t to uint16_t, expanding the range of valid module IDs. Additionally, a new method, isModEnabled, is declared and implemented in UsermodManager to check the enabled state of a specific usermod by its ID.

Changes

File(s)	Change Summary
wled00/fcn_declare.h	Added virtual method Usermod::isEnabled() (returns true by default). Changed getUMData parameter type from uint8_t to uint16_t. Declared new function UsermodManager::isModEnabled(uint16_t mod_id = USERMOD_ID_RESERVED).
wled00/um_manager.cpp	Updated getUMData to use uint16_t for mod_id. Added implementation for isModEnabled(uint16_t mod_id), which checks if a usermod is enabled by ID.

---

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 10b925a and 18a110f.

📒 Files selected for processing (2)

• wled00/fcn_declare.h (3 hunks)
• wled00/um_manager.cpp (1 hunks)

🧰 Additional context used

🧬 Code Graph Analysis (1)

wled00/um_manager.cpp (2)

wled00/fcn_declare.h (4)

• getUMData (454-454)
• data (419-419)
• data (419-419)
• isModEnabled (474-474)

usermods/audioreactive/audio_reactive.cpp (2)

• data (1438-1443)
• data (1438-1438)

🪛 Cppcheck (2.10-2)

wled00/um_manager.cpp

[error] 35-35: Comparing pointers that point to different objects

(comparePointers)

---

[error] 42-42: Comparing pointers that point to different objects

(comparePointers)

🔇 Additional comments (5)

wled00/fcn_declare.h (3)

433-433: New method provides a standardized way to check usermod enabled state.

Adding a virtual isEnabled() method with a default return value of true is a good design pattern. It allows derived usermods to override this behavior while providing sensible default behavior.

---

454-454: Parameter type standardization improves consistency.

Changing the mod_id parameter type from uint8_t to uint16_t increases the range of possible usermod IDs and makes the interface more consistent with other methods that use uint16_t for module identification.

---

474-474: Added utility method complements the new isEnabled interface.

The new isModEnabled method provides a convenient way to check if a specific usermod is enabled without needing direct access to the usermod instance. This is a helpful addition that complements the new isEnabled() virtual method.

wled00/um_manager.cpp (2)

34-40: Parameter type updated for consistency.

The implementation of getUMData now correctly uses uint16_t for the mod_id parameter, matching the declaration in the header file. This change allows for a wider range of module IDs.

🧰 Tools

🪛 Cppcheck (2.10-2)

[error] 35-35: Comparing pointers that point to different objects

(comparePointers)

---

41-47: Implementation of isModEnabled is efficient and well-structured.

The implementation iterates through registered usermods and returns the enabled state of the first matching usermod by ID. The logic follows the same filtering pattern used in getUMData and returns false if no matching usermod is found.

The function correctly handles both:

1. Checking a specific usermod (when mod_id > 0)
2. Checking the first usermod (when mod_id == USERMOD_ID_RESERVED)

🧰 Tools

🪛 Cppcheck (2.10-2)

[error] 42-42: Comparing pointers that point to different objects

(comparePointers)

✨ Finishing Touches

• 📝 Generate Docstrings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[blazoncek]

Thank you for the contribution.
What is the benefit in your opinion (for current state and the future)?

For enabled feature I would expect modification of all existing usermods that employ enabled functionality. It would also make sense to add private variable to base class if this is something that's beneficial.

[rhkean]

Thank you for the contribution. What is the benefit in your opinion (for current state and the future)?

I'm currently working on 2 mods (Enable/Disable WiFi and BLE). one of which depends on the other. Since 'enabled' is a simple bool check, it seemed more practical to add a public method to check if a mod was enabled rather than all the code needed (and probably binary size) for the getUMData call

For enabled feature I would expect modification of all existing usermods that employ enabled functionality. It would also make sense to add private variable to base class if this is something that's beneficial.

by "I would expect" do you mean that I should include the modifications to the other usermods in this PR? I don't mind doing that. I wasn't certain on the communitie's preferences to modifying other contributors mods.

[blazoncek]

I don't think enabling/disabling WiFi should be a usermod. In fact I've added a short snippet into JSON handling to start/stop AP mode on JSON API request. See json.cpp, deserializeState(). It would be better to enhance that, but be prepared that I will comment heavily (with good intentions even if it might nod sound so).
And IMO BLE has a toll that we won't be willing to pay, but as a usermod it might have a chance. So far no-one brought it to living.

I should include the modifications to the other usermods in this PR

Yes, if you are adding a feature, make sure it is used where appropriate. Some usermods indeed have enabled() public method and ability to detect if usermod is available is already included using lookup() function.
There are a few usermods that already share data via public methods. getUMData() was intended to share data with core WLED features but I do not think we'll pursue that further as other approaches were made (i.e. ability to add usermod effects).

[rhkean]

I don't think enabling/disabling WiFi should be a usermod. In fact I've added a short snippet into JSON handling to start/stop AP mode on JSON API request. See json.cpp, deserializeState(). It would be better to enhance that, but be prepared that I will comment heavily (with good intentions even if it might nod sound so). And IMO BLE has a toll that we won't be willing to pay, but as a usermod it might have a chance. So far no-one brought it to living.

interestingly, I came to that same conclusion today. LOL
I am actually almost finished re-writing my disable/Enable WiFi as a core feature. I've finished the code modifications. Now I'm working on the WiFi settings page updates.

The BLE mod that I'm writing will use BLE advertising, so a paired connection should not be needed.... I'll keep it as a separate PR, though. The trick is in managing the radio which is why I had to back up and implement the wifi-disable first.

I should include the modifications to the other usermods in this PR

Yes, if you are adding a feature, make sure it is used where appropriate. Some usermods indeed have enabled() public method and ability to detect if usermod is available is already included using lookup() function. There are a few usermods that already share data via public methods. getUMData() was intended to share data with core WLED features but I do not think we'll pursue that further as other approaches were made (i.e. ability to add usermod effects).

understood. I'll make those changes and update the PR

[willmmiles]

I'm not sure that we need this to be a global feature. Every currently merged usermod that interacts with another usermod does so by either (a) making custom calls directly on the other usermod's object, or (b) uses getUMData(). If you're implementing a specialized communication between two modules, I'd recommend the former approach. If you're instead trying to ask "does this usermod exist in this build", I would recommend a compile-time test; see https://github.com/wled/WLED/blob/354a0aef5205c2c56f4784a554d85eeb84972ec0/usermods/PWM_fan/setup_deps.py for an example of how to do that.

That said: if we do want to adopt an "enabled" state as a queryable property on all usermods, I think we should adopt MoonModules' approach to improve compatibility between the forks. https://github.com/MoonModules/WLED-MM/blob/7cb8eebba61e0e14f15cbf036f68f6030e9f5ca0/wled00/fcn_declare.h#L301C2-L302C32 (I was pretty sure I'd seen a PR for this, but I can't find it now -- maybe it was just a suggestion on the Discord?)

[rhkean]

The idea was to test if an installed mod (that could be enabled or disabled) was enabled in a simpler way than using the getUMData() method. That felt like a lot of code to write to a) make a simple bool available to other usermods. It makes sense to me if you're needing access to a large set of data like with the AR mod, but for a simple bool test.

If you all don't see it as a value-add, I can see your point of view. We can close the PR, if you want.

[willmmiles]

I agree getUMdata isn't a good solution for "simple" requests. The common solution in current use is static_cast<UsermodMyOtherClass*>(UsermodManager::lookup(USERMOD_ID_MYOTHERUSERMOD))->getDataIWant(), which works because most usermods know exactly which other modules they want to interact with. I believe the use case you described above falls in to this class -- your second module knows exactly which other module you want to interact with, so you're free to use lookup() to make calls directly on the module's own class type.

What I'm uncertain about is the use case for "the set of modules that expose an enable", especially since in this fork there's no API to do pretty much anything with them in a generic way on them except get their ID code. (The MM fork also requires each usermod provide a name, which permits making a generic UI to manipulate the enable setting.) Can you help me understand what is the use case for this set that isn't served by lookup()?

[rhkean]

Can you help me understand what is the use case for this set that isn't served by lookup()?

Nope.... LOL
you make great points. I cannot see any advantage to this mod after this conversation... no speed or code size advantages, either.

Thank you for useful feedback and great conversation. If you agree, I'll close this PR and focus on disabling WiFi... LOL