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.

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

xml comma bugfix + few string optimisations #4286

Merged
merged 3 commits into from
Nov 28, 2024

Conversation

blazoncek
Copy link
Collaborator

fix for incorrect JSON array creation.

@netmindz
Copy link
Collaborator

Can you please provide a little more info on what what the impact of this issue was so others know how to test your fix and also establish if it needs to be merged before the release candidate is tagged

@blazoncek
Copy link
Collaborator Author

Sorry.
The JSON array for reserved pins (settings page) was incorrectly constructed (wrong order of commas) in some cases (Ethernet). It should be included in 0.15.0 release.
I also added a few string optimisations to reduce image size.

@netmindz
Copy link
Collaborator

Thank you for that further information, but I must confess that I am still unclear on what was wrong or how to reproduce the error. I find examples really helpful

@willmmiles
Copy link
Collaborator

willmmiles commented Nov 16, 2024

Thank you for that further information, but I must confess that I am still unclear on what was wrong or how to reproduce the error. I find examples really helpful

The bug is a code inconsistency on whether the commas in the reserved pin list ought to be added first or last. The "disallowed" pins added commas after, but other cases (DMX, hardwareTx, wired Ethernet) would add a comma first, so the list would end up reading d.rsvd=[1,2,,3,4,5] -- note extra comma in the middle. To reproduce the issue, make a build with WLED_ENABLE_DMX, WLED_DEBUG, or WLED_USE_ETHERNET and inspect the settings output, eg. http://wled.local/settings/s.js?p=2

That said: while not technically correct, in my prior testing, the UI did seem to handle the empty element gracefully.

Copy link
Collaborator

@willmmiles willmmiles left a comment

Choose a reason for hiding this comment

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

Generally looks good to me and passes my functional tests.

wled00/xml.cpp Show resolved Hide resolved
wled00/xml.cpp Show resolved Hide resolved
wled00/xml.cpp Outdated Show resolved Hide resolved
@blazoncek blazoncek changed the title xml comma bugfix + few optimisations xml comma bugfix + few string optimisations Nov 28, 2024
@netmindz netmindz added this to the 0.15.0-final candidate milestone Nov 28, 2024
@willmmiles willmmiles merged commit a121f5b into Aircoookie:0_15 Nov 28, 2024
20 checks passed
@blazoncek blazoncek deleted the xml-bugfix branch November 29, 2024 20:25
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.

3 participants