Skip to content

Allow product information to be specified at build time - #3441

Closed
Moustachauve wants to merge 2 commits into
wled:0_14_1from
Moustachauve:product-data
Closed

Allow product information to be specified at build time#3441
Moustachauve wants to merge 2 commits into
wled:0_14_1from
Moustachauve:product-data

Conversation

@Moustachauve

Copy link
Copy Markdown
Contributor

This allow people who make custom builds of WLED (Ex: QuinLED) to specify the details about the product at build time rather than having it hardcoded. The default values of WLED are unchanged.

This is similar to #3407 and has a similar end goal.

This allow people who make custom builds of WLED (Ex: QuinLED) to specify the details about the product at build time rather than having it hardcoded.
The default values of WLED are unchanged.

@blazoncek blazoncek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO the goal is OK but the implementation is not. Changing existing behaviour will break functionality at some point.
I think a better approach would be a new API for this specific purpose.

Comment thread wled00/wled.h Outdated
Comment thread usermods/PIR_sensor_switch/usermod_PIR_sensor_switch.h Outdated
@Moustachauve

Copy link
Copy Markdown
Contributor Author

IMO the goal is OK but the implementation is not. Changing existing behaviour will break functionality at some point. I think a better approach would be a new API for this specific purpose.

The existing behavior is not changed at all in my opinion. The API result will be exactly the same as before.
3rd party devices could already change that value at compile time if they wanted (that's why the value is in the API afterall)

@blazoncek

Copy link
Copy Markdown
Contributor

Unfortunately I cannot comment the use in JSON API as I do not know what was original purpose. I also do not know which 3rd party systems use these fields for their purpose (HomeAssistant and possibly others?) in determining connected systems. Please research which 3rd party systems use "mf" and "mdl" JSON API fields. Perhaps @Aircoookie can shed some more light when he returns.

Still, I cannot approve the change in MQTT as I know the fields are needed for correct classification of HomeAssistant devices detected. This will certainly break existing configurations.

@Moustachauve

Copy link
Copy Markdown
Contributor Author

I would argue that for most people editing these fields, making sure these are not detected as WLED would be a benefit and not a bug.

@Moustachauve

Copy link
Copy Markdown
Contributor Author

I also do not know which 3rd party systems use these fields for their purpose (HomeAssistant and possibly others?) in determining connected systems. Please research which 3rd party systems use "mf" and "mdl" JSON API fields.

mf and mdl are only in usermods. No third party rely on it for 99% of the cases. These values are already modifiable. Why prevent people from modifying them if they want to? This does not affect default builds of WLED in any way. I can add more documentation to say to be careful when modifying these values but that's not something default WLED ever has to worry about.

Still, I cannot approve the change in MQTT as I know the fields are needed for correct classification of HomeAssistant devices detected. This will certainly break existing configurations.

I just checked both in the code of Home Assistant and by testing with my own instance of HA and I am happy to report to you that Home Assistant does not use MQTT at all for WLED and also the device has full functionality.

Add some warning in the comments above the constants so that people can be aware of the risks when changing this value.
@blazoncek
blazoncek changed the base branch from main to 0_14_1 October 17, 2023 04:38

@blazoncek blazoncek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants