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

Add BaseConfigUpdateEvent and handle base configuration updates #747

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
172 changes: 102 additions & 70 deletions ledfx/api/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from ledfx.api import RestEndpoint
from ledfx.api.utils import PERMITTED_KEYS
from ledfx.config import (
CORE_CONFIG_KEYS_NO_RESTART,
CORE_CONFIG_SCHEMA,
WLED_CONFIG_SCHEMA,
create_backup,
Expand All @@ -17,6 +18,7 @@
from ledfx.consts import CONFIGURATION_VERSION
from ledfx.effects.audio import AudioInputSource
from ledfx.effects.melbank import Melbanks
from ledfx.events import BaseConfigUpdateEvent

_LOGGER = logging.getLogger(__name__)

Expand Down Expand Up @@ -181,85 +183,115 @@ async def put(self, request: web.Request) -> web.Response:
Returns:
web.Response: The HTTP response object
"""

try:
config = await request.json()
except JSONDecodeError:
return await self.json_decode_error()
except Exception as e:
return await self.generic_error(str(e))

audio_config = validate_and_trim_config(
config.pop("audio", {}),
AudioInputSource.AUDIO_CONFIG_SCHEMA.fget(),
"audio",
)
wled_config = validate_and_trim_config(
config.pop("wled_preferences", {}),
WLED_CONFIG_SCHEMA,
"wled_preferences",
)
melbanks_config = validate_and_trim_config(
config.pop("melbanks", {}), Melbanks.CONFIG_SCHEMA, "melbanks"
try:
self.update_config(config)
self._ledfx.events.fire_event(BaseConfigUpdateEvent(config))
save_config(
config=self._ledfx.config, config_dir=self._ledfx.config_dir
)
core_config = validate_and_trim_config(
config, CORE_CONFIG_SCHEMA, "core"
need_restart = self.check_need_restart(config)
if need_restart:
# Ugly - return success to frontend before restarting
try:
return await self.request_success(
type="success",
message="LedFx is restarting to apply the new configuration",
)
finally:
self._ledfx.loop.call_soon_threadsafe(self._ledfx.stop, 4)
return await self.request_success(
type="success", message="Configuration Updated"
)
except (KeyError, vol.MultipleInvalid) as msg:
error_message = f"Error updating config: {msg}"
_LOGGER.warning(error_message)
return await self.invalid_request(error_message)
except Exception as e:
Comment on lines +186 to +217
Copy link
Contributor

Choose a reason for hiding this comment

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

The modifications to the put method to handle configuration updates and potential restarts are well-implemented. The method now correctly updates the configuration, fires a BaseConfigUpdateEvent, and checks if a restart is necessary based on the updated configuration. This aligns with the PR's objectives to enhance configuration management and minimize disruptions caused by restarts. However, consider adding more detailed logging for each step to improve traceability and debugging.

+ _LOGGER.info("Updating LedFx configuration.")
+ _LOGGER.info("Firing BaseConfigUpdateEvent.")
+ _LOGGER.info("Checking if restart is needed.")

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
try:
config = await request.json()
except JSONDecodeError:
return await self.json_decode_error()
except Exception as e:
return await self.generic_error(str(e))
audio_config = validate_and_trim_config(
config.pop("audio", {}),
AudioInputSource.AUDIO_CONFIG_SCHEMA.fget(),
"audio",
)
wled_config = validate_and_trim_config(
config.pop("wled_preferences", {}),
WLED_CONFIG_SCHEMA,
"wled_preferences",
)
melbanks_config = validate_and_trim_config(
config.pop("melbanks", {}), Melbanks.CONFIG_SCHEMA, "melbanks"
try:
self.update_config(config)
self._ledfx.events.fire_event(BaseConfigUpdateEvent(config))
save_config(
config=self._ledfx.config, config_dir=self._ledfx.config_dir
)
core_config = validate_and_trim_config(
config, CORE_CONFIG_SCHEMA, "core"
need_restart = self.check_need_restart(config)
if need_restart:
# Ugly - return success to frontend before restarting
try:
return await self.request_success(
type="success",
message="LedFx is restarting to apply the new configuration",
)
finally:
self._ledfx.loop.call_soon_threadsafe(self._ledfx.stop, 4)
return await self.request_success(
type="success", message="Configuration Updated"
)
except (KeyError, vol.MultipleInvalid) as msg:
error_message = f"Error updating config: {msg}"
_LOGGER.warning(error_message)
return await self.invalid_request(error_message)
except Exception as e:
try:
config = await request.json()
except JSONDecodeError:
return await self.json_decode_error()
except Exception as e:
return await self.generic_error(str(e))
try:
_LOGGER.info("Updating LedFx configuration.")
self.update_config(config)
_LOGGER.info("Firing BaseConfigUpdateEvent.")
self._ledfx.events.fire_event(BaseConfigUpdateEvent(config))
save_config(
config=self._ledfx.config, config_dir=self._ledfx.config_dir
)
_LOGGER.info("Checking if restart is needed.")
need_restart = self.check_need_restart(config)
if need_restart:
# Ugly - return success to frontend before restarting
try:
return await self.request_success(
type="success",
message="LedFx is restarting to apply the new configuration",
)
finally:
self._ledfx.loop.call_soon_threadsafe(self._ledfx.stop, 4)
return await self.request_success(
type="success", message="Configuration Updated"
)
except (KeyError, vol.MultipleInvalid) as msg:
error_message = f"Error updating config: {msg}"
_LOGGER.warning(error_message)
return await self.invalid_request(error_message)
except Exception as e:

return await self.internal_error(str(e))

self._ledfx.config["audio"].update(audio_config)
self._ledfx.config["melbanks"].update(melbanks_config)
self._ledfx.config.update(core_config)
def update_config(self, config):
"""
Updates the configuration of the LedFx instance with the provided config.

# handle special case wled_preferences nested dict
for key in wled_config:
if key in self._ledfx.config["wled_preferences"]:
self._ledfx.config["wled_preferences"][key].update(
wled_config[key]
)
else:
self._ledfx.config["wled_preferences"][key] = wled_config[
key
]

# TODO
# Do something if wled preferences config is updated

if (
hasattr(self._ledfx, "audio")
and self._ledfx.audio is not None
and audio_config
):
self._ledfx.audio.update_config(self._ledfx.config["audio"])

if hasattr(self._ledfx, "audio") and melbanks_config:
self._ledfx.audio.melbanks.update_config(
self._ledfx.config["melbanks"]
)
Args:
config (dict): The new configuration to be applied.

if core_config and not (
any(
key in core_config
for key in [
"global_brightness",
"create_segments",
"scan_on_startup",
"user_presets",
"transmission_mode",
# ToDo:
# temporary let ledfx restart when visualisation_maxlen is changed
# until backend can change the length of the pixel data sent via websocket
# "visualisation_maxlen",
]
)
and len(core_config) == 1
):
self._ledfx.loop.call_soon_threadsafe(self._ledfx.stop, 4)
Returns:
None
"""
audio_config = validate_and_trim_config(
config.pop("audio", {}),
AudioInputSource.AUDIO_CONFIG_SCHEMA.fget(),
"audio",
)
wled_config = validate_and_trim_config(
config.pop("wled_preferences", {}),
WLED_CONFIG_SCHEMA,
"wled_preferences",
)
melbanks_config = validate_and_trim_config(
config.pop("melbanks", {}), Melbanks.CONFIG_SCHEMA, "melbanks"
)
core_config = validate_and_trim_config(
config, CORE_CONFIG_SCHEMA, "core"
)

save_config(
config=self._ledfx.config,
config_dir=self._ledfx.config_dir,
self._ledfx.config["audio"].update(audio_config)
self._ledfx.config["melbanks"].update(melbanks_config)
self._ledfx.config.update(core_config)

# handle special case wled_preferences nested dict
for key in wled_config:
if key in self._ledfx.config["wled_preferences"]:
self._ledfx.config["wled_preferences"][key].update(
wled_config[key]
)
else:
self._ledfx.config["wled_preferences"][key] = wled_config[key]

if (
hasattr(self._ledfx, "audio")
and self._ledfx.audio is not None
and audio_config
):
self._ledfx.audio.update_config(self._ledfx.config["audio"])

if hasattr(self._ledfx, "audio") and melbanks_config:
self._ledfx.audio.melbanks.update_config(
self._ledfx.config["melbanks"]
)
return await self.request_success()

except JSONDecodeError:
return await self.json_decode_error()
if core_config:
self._ledfx.events.fire_event(BaseConfigUpdateEvent(config))

except (KeyError, vol.MultipleInvalid) as msg:
error_message = f"Error updating config: {msg}"
_LOGGER.warning(error_message)
return await self.internal_error("error", error_message)
def check_need_restart(self, config):
"""
Checks if a restart is needed based on the provided configuration.

Args:
config (dict): The configuration to be checked.

Returns:
bool: True if a restart is needed, False otherwise.
"""
core_config = validate_and_trim_config(
config, CORE_CONFIG_SCHEMA, "core"
)

# If core_config is empty, no restart is needed
if not core_config:
return False

need_restart = True
if any(key in core_config for key in CORE_CONFIG_KEYS_NO_RESTART):
need_restart = False

return need_restart
Comment on lines +275 to +297
Copy link
Contributor

Choose a reason for hiding this comment

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

The check_need_restart method correctly assesses whether a restart is necessary based on the provided configuration. It uses CORE_CONFIG_KEYS_NO_RESTART to determine if any of the updated keys allow for dynamic updates without a restart. This logic is essential for minimizing disruptions. However, consider enhancing this method to log which specific configuration changes are triggering a restart, as this could aid in debugging and user understanding.

+ _LOGGER.debug(f"Checking need for restart with updated config keys: {list(config.keys())}")
+ if need_restart:
+     _LOGGER.info("A restart is required due to changes in the core configuration.")
+ else:
+     _LOGGER.info("No restart is required for the applied configuration changes.")

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
def check_need_restart(self, config):
"""
Checks if a restart is needed based on the provided configuration.
Args:
config (dict): The configuration to be checked.
Returns:
bool: True if a restart is needed, False otherwise.
"""
core_config = validate_and_trim_config(
config, CORE_CONFIG_SCHEMA, "core"
)
# If core_config is empty, no restart is needed
if not core_config:
return False
need_restart = True
if any(key in core_config for key in CORE_CONFIG_KEYS_NO_RESTART):
need_restart = False
return need_restart
def check_need_restart(self, config):
"""
Checks if a restart is needed based on the provided configuration.
Args:
config (dict): The configuration to be checked.
Returns:
bool: True if a restart is needed, False otherwise.
"""
core_config = validate_and_trim_config(
config, CORE_CONFIG_SCHEMA, "core"
)
# If core_config is empty, no restart is needed
if not core_config:
return False
need_restart = True
if any(key in core_config for key in CORE_CONFIG_KEYS_NO_RESTART):
need_restart = False
_LOGGER.debug(f"Checking need for restart with updated config keys: {list(config.keys())}")
if need_restart:
_LOGGER.info("A restart is required due to changes in the core configuration.")
else:
_LOGGER.info("No restart is required for the applied configuration changes.")
return need_restart

15 changes: 15 additions & 0 deletions ledfx/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,21 @@
"DELETE": "Config deleted. Backup Created.",
}

# Some core config keys that don't need a restart to take effect - list them here to use elsewhere
CORE_CONFIG_KEYS_NO_RESTART = [
"global_brightness",
"create_segments",
"scan_on_startup",
"user_presets",
"visualisation_maxlen",
"visualisation_fps",
]
# Collection of keys that are used for visualisation configuration - used to check if we need to restart the visualisation event listeners
VISUALISATION_CONFIG_KEYS = [
"visualisation_fps",
"visualisation_maxlen",
]


# Transmission types for pixel visualisation on frontend
class Transmission:
Expand Down
34 changes: 29 additions & 5 deletions ledfx/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
validate_gradient,
)
from ledfx.config import (
VISUALISATION_CONFIG_KEYS,
Transmission,
create_backup,
get_ssl_certs,
Expand Down Expand Up @@ -74,6 +75,7 @@ def __init__(
clear_config=False,
clear_effects=False,
):

self.icon = icon
self.config_dir = config_dir

Expand Down Expand Up @@ -118,11 +120,27 @@ def __init__(
self.setup_logqueue()
self.events = Events(self)
self.setup_visualisation_events()
self.events.add_listener(
self.handle_base_configuration_update, Event.BASE_CONFIG_UPDATE
)
self.http = HttpServer(
ledfx=self, host=self.host, port=self.port, port_s=self.port_s
)
self.exit_code = None

def handle_base_configuration_update(self, event):
"""
Handles the update of the base configuration where there are specific things that need to be done.

Currently, only visualisation configuration is handled this way, since they require the creation of new event listeners.

Args:
event (Event): The event that triggered the update - this will always be a BaseConfigUpdateEvent.
"""

if any(key in self.config for key in VISUALISATION_CONFIG_KEYS):
self.setup_visualisation_events()

def dev_enabled(self):
return self.config["dev_mode"]

Expand Down Expand Up @@ -168,6 +186,12 @@ def setup_visualisation_events(self):
creates event listeners to fire visualisation events at
a given rate
"""
# Remove existing listeners if they exist
if hasattr(self, "visualisation_update_listener"):
self.visualisation_update_listener = None
self.virtual_listener()
self.device_listener()

min_time_since = 1 / self.config["visualisation_fps"]
time_since_last = {}
max_len = self.config["visualisation_maxlen"]
Expand Down Expand Up @@ -208,13 +232,13 @@ def handle_visualisation_update(event):
VisualisationUpdateEvent(is_device, vis_id, pixels)
)

self.events.add_listener(
handle_visualisation_update,
self.visualisation_update_listener = handle_visualisation_update
self.virtual_listener = self.events.add_listener(
self.visualisation_update_listener,
Event.VIRTUAL_UPDATE,
)

self.events.add_listener(
handle_visualisation_update,
self.device_listener = self.events.add_listener(
self.visualisation_update_listener,
Event.DEVICE_UPDATE,
)

Expand Down
11 changes: 11 additions & 0 deletions ledfx/events.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
class Event:
"""Base for events"""

BASE_CONFIG_UPDATE = "base_config_update"
LEDFX_SHUTDOWN = "shutdown"
DEVICE_CREATED = "device_created"
DEVICE_UPDATE = "device_update"
Expand Down Expand Up @@ -166,6 +167,16 @@ def __init__(self, virtual_id, config):
self.config = config


class BaseConfigUpdateEvent(Event):
"""
Event emitted when an item in the base configuration is updated.
"""

def __init__(self, config):
super().__init__(Event.BASE_CONFIG_UPDATE)
self.config = config


class LedFxShutdownEvent(Event):
"""Event emitted when LedFx is shutting down"""

Expand Down
Loading