Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Sep 28, 2025

Fixed Bus Type Change Restrictions with Reboot Handling

Successfully implemented the solution to allow bus type changes while prompting for reboot instead of restricting them, with all reviewer feedback addressed.

Changes Completed:

  • Remove restriction logic - modified line 273 in settings_leds.htm to remove s.disabled = true for non-last buses
  • Add busChanged variable - added tracking variables busChanged, originalBusTypes
  • Implement change detection - added captureInitialBusState() and checkBusChanges() functions
  • Track bus changes - checkBusChanges() is called when:
    • Bus type is changed via onchange="checkBusChanges();UI(true)"
    • Bus count changes via addLEDs() function modification
  • Add reboot warning UI - modified trySubmit() to show confirmation dialog when busChanged=true
  • Handle save action - user can proceed (with reboot) or cancel when bus changes detected
  • Server-side handling - added RBT parameter handling in set.cpp for SUBPAGE_LEDS
  • Address all review feedback:
    • Removed redundant bus count check (bus type changes catch additions/removals)
    • Exempt virtual and network buses from reboot requirement
    • Shortened confirmation dialog message for better UX
    • Cleaned up unused originalBusCount variable
  • Test manually - verified UI works correctly, bus type dropdowns are no longer disabled
  • Validate build - web UI builds successfully, all tests pass

Technical Details:

Frontend Changes (settings_leds.htm):

  • Removed if (i < LTs.length-1) s.disabled = true; restriction
  • Added state tracking variables and functions
  • Enhanced form submission to detect changes and prompt for reboot
  • Code cleanup: Streamlined change detection to only track bus types (removed unused originalBusCount)
  • Smart filtering: Only physical hardware buses trigger reboot, virtual/network buses exempted
  • Added hidden RBT=1 field when reboot is required

Backend Changes (set.cpp):

  • Added reboot handling: if (request->hasArg(F("RBT"))) doReboot = true; in SUBPAGE_LEDS

User Experience:

  • All bus type selects are now enabled and editable
  • Virtual and network buses don't trigger reboot requirements (hardware-independent)
  • When physical bus types change, user gets a concise confirmation dialog
  • User can choose to proceed (with reboot) or cancel the changes
  • System automatically reboots after saving when required
  • Clean, optimized code without unused variables

This resolves the confusing restriction behavior and provides a user-friendly approach to handling hardware changes that require reboot, with improved logic and clean code structure based on all reviewer feedback.

Original prompt

This section details on the original issue you should resolve

<issue_title>Reboot instead of restricting LED-type change with multiple buses</issue_title>
<issue_description>When adding more than one bus in LED settings, editing the type is only possible for the last bus.
This is to ensure proper hardware initialisation.
However, this is confusing to users and should be handled differently. Instead of restricting it in the settings, a variable should be added to the LED settings page that keeps track of the problematic changes i.e. the bus type and if necessary, display a warning and issue a reboot.
@willmmiles do you know which settings or combinations are critical in this regard?</issue_description>

<agent_instructions>check current code, issuing a reboot from the web client is already implemented, so it the same way.
add a variable "busChanged" to the led settings page which is set to true a bus is added or a bus type is changed. when saving the led settings, inform the user that a reboot is required, let the user decide to proceed or cancel. </agent_instructions>

Comments on the Issue (you are @copilot in this section)

Fixes #4958

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@Copilot Copilot AI changed the title [WIP] Reboot instead of restricting LED-type change with multiple buses Remove bus type restrictions and add reboot handling for LED settings Sep 28, 2025
@Copilot Copilot AI requested a review from DedeHai September 28, 2025 06:49
Copilot finished work on behalf of DedeHai September 28, 2025 06:49
Copy link
Collaborator

@DedeHai DedeHai left a comment

Choose a reason for hiding this comment

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

fix this.

function checkBusChanges() {
// check if bus configuration has changed
let buses = d.Sf.querySelectorAll("#mLC select[name^=LT]");
let currentBusCount = buses.length;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is redundant, if bus count increases, the below check for value will catch it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the redundant bus count check. The bus type comparison now catches both type changes and additions/removals. Fixed in commit 7dba246.


// check if any bus type changed
for (let i = 0; i < buses.length; i++) {
if (parseInt(buses[i].value) != originalBusTypes[i]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

exempt virtual and network buses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added exemption for virtual and network buses using isVir() and isNet() functions. These buses are hardware-independent and don't require reboot. Fixed in commit 7dba246.

// check for bus changes that require reboot
checkBusChanges();
if (busChanged) {
var msg = "LED hardware configuration changed. A reboot is required for changes to take effect.\n\nDo you want to continue and reboot after saving?";
Copy link
Collaborator

Choose a reason for hiding this comment

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

shorten the string but keep the meaning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shortened the confirmation message to "LED hardware changed. Reboot required to apply changes.\n\nContinue and reboot after saving?" while keeping the meaning clear. Fixed in commit 7dba246.

@DedeHai
Copy link
Collaborator

DedeHai commented Sep 28, 2025

@copilot please fix the review comments.

…tual/network buses, shorten dialog message

Co-authored-by: DedeHai <[email protected]>
Copy link
Contributor Author

Copilot AI commented Sep 28, 2025

@copilot please fix the review comments.

All review comments have been addressed in commit 7dba246:

  • Removed redundant bus count check
  • Exempted virtual/network buses from reboot requirement
  • Shortened confirmation dialog message

LED Settings UI

@Copilot Copilot AI requested a review from DedeHai September 28, 2025 10:59
Copilot finished work on behalf of DedeHai September 28, 2025 10:59
@DedeHai
Copy link
Collaborator

DedeHai commented Sep 28, 2025

@copilot originalBusCount is now unused

Copy link
Contributor Author

Copilot AI commented Sep 28, 2025

@copilot originalBusCount is now unused

...

Removed the unused originalBusCount variable. Only tracking originalBusTypes array is needed since bus type comparison handles both type changes and bus additions/removals. Fixed in commit 666df74.

Copilot finished work on behalf of DedeHai September 28, 2025 12:45
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.

Reboot instead of restricting LED-type change with multiple buses
2 participants