Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[RF] Proposal fix for issue #2139 + Extra documentation #2157

Open
wants to merge 2 commits into
base: development
Choose a base branch
from
Open

[RF] Proposal fix for issue #2139 + Extra documentation #2157

wants to merge 2 commits into from
+483 −184

Conversation

Odyno
Copy link
Contributor

@Odyno Odyno commented Jan 29, 2025 •
edited
Loading

Description:

This pull request proposes a fix for issue #2139 and includes several improvements and enhancements to the RF module.

Changes:

  1. Conditional MQTT Message Sending:
    • Introduced two #define directives to allow users to choose which MQTT message to send for each RF signal received. Users can uncomment the desired line to enable the corresponding functionality.
#define RF_on_HAS_as_DeviceTrigger //uncomment this line so as to create a Home Assistant device trigger for each RF signal received
//#define RF_on_HAS_as_MQTTSencor //uncomment this line so as to create a Home Assistant MQTT sensor for each RF signal received
  1. Refresh of announceDeviceTrigger Message:
  • Updated the announceDeviceTrigger message to align with Home Assistant specifications. This change does not affect functionality but ensures compliance with the latest standards.
  1. Reformatting and Additional Documentation:
  • Reformatted the RF module code for better readability and maintainability + functions documentation
  • Added extra documentation to clarify the functionality and usage of the RF module on Site
  1. Improved Logging for RF Handling:
  • Enhanced logging to provide more detailed insights and debugging information for RF signal handling.

Impact:

These changes improve the flexibility and usability of the RF module, allowing users to customize their Home Assistant integration more easily. With this implementaion can be a broken retroconpatibilty

The enhanced logging and documentation will aid in troubleshooting and understanding the RF module's behavior.

Base for Future Work

The idea is to move the configuration to Home Assistant using MQTT Selectors. This will allow users to enable or disable modules, set the current frequency, and choose the type of message to send, all through MQTT Selectors.

Please review the proposed changes and provide feedback. Thank you!

Checklist:

  • The pull request is done against the latest development branch
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • I accept the DCO.

@Odyno
Copy link
Contributor Author

Odyno commented Jan 29, 2025

There appears to be some confusion regarding how the RF module is announced on Home Assistant. I have created this pull request as a preview of the work I am doing to address issue #2139. I am currently reviewing the RF module to clean up the code and improve the documentation. My suggestion is to test these changes for a couple of days to see if we can further enhance the documentation and user interaction.

@Odyno Odyno changed the title Proposal for #2139 + Restile of announceDeviceTrigger message [RF] Proposal fix for issue #2139 + Extra documentation Jan 29, 2025
@Odyno
Refactor announceDeviceTrigger function to improve parameter handling…
b18c4db 
… and documentation for optional device information

Fix null pointer references in RFtoMQTTdiscovery and update logging levels for RF signal handling
Refactor RF to MQTT discovery functions for improved clarity and parameter handling
@Odyno Odyno marked this pull request as ready for review February 9, 2025 19:10
@Odyno
Copy link
Contributor Author

Odyno commented Feb 9, 2025

@1technophile and all other contributors, I have completed a review of the code and documentation for the RF module. Please feel free to provide any comments and let me know if you would like to merge it into the master branch. Thank you!

@1technophile
Copy link
Owner

Thanks for the PR, give me a few days to review it

1technophile
1technophile reviewed Feb 25, 2025
View reviewed changes
main/ZgatewayRF.ino
Comment on lines +398 to +400
float rfFrequency = RFConfig.frequency;
int rfReceiverGPIO = RF_RECEIVER_GPIO;
int rfEmitterGPIO = RF_EMITTER_GPIO;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason for not using directly the macros inside the logs and avoid adding new variables ?

1technophile
1technophile reviewed Feb 25, 2025
View reviewed changes
main/ZgatewayRF.ino
disableCurrentReceiver();
# ifdef ZradioCC1101
initCC1101();
int txPower = RFdata["txpower"] | RF_CC1101_TXPOWER;
ELECHOUSE_cc1101.setPA((int)txPower);
Log.notice(F("CC1101 TX Power: %d" CR), txPower);
Log.notice(F("[RF] CC1101 TX Power: %d" CR), txPower);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea to prefix with the module, but for consistency we should add it to the other modules

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe in another PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@1technophile 1technophile 1technophile left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Odyno @1technophile