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

De-duplicate Ambient LED management code #5156

Merged

Conversation

fifieldt
Copy link
Contributor

We currently have 4 different places where we have the logic for modifying LED state of the various types of Ambient LEDs, ExternalNotificationModule::SetExternalOff
ExternalNotificationModule::SetExternalOn
AmbientLightingThread::setLighting
AmbientLightingThread::setLightingOff

This patch de-duplicates the methods in ExternalNotification to a single method, using a boolean to toggle whether we're turning things on or off.

@fifieldt fifieldt force-pushed the external-notification-cleanup branch from d015074 to 64ab6be Compare October 27, 2024 02:26
@fifieldt
Copy link
Contributor Author

Github diff doesn't make it easy to see: here's past off and on:
image

image

@fifieldt
Copy link
Contributor Author

Where I think this could go next -- make AmbientLighting responsible for LED stuff and call it from ExternalNotification.

@fifieldt fifieldt force-pushed the external-notification-cleanup branch 2 times, most recently from 174570f to e38e5d0 Compare October 28, 2024 03:44
@caveman99
Copy link
Member

caveman99 commented Oct 29, 2024

Where I think this could go next -- make AmbientLighting responsible for LED stuff and call it from ExternalNotification.

We are trying to resolve those cross-depencencies of the modules. In principal it is good but can we use the notification system we have in the firmware for IPC. E.G. Ambient lighting offering a subscription service for LED changes and the External Notifications just using it (if available)

You can look at how it's done for notifying others about GPS position changes :-)

We currently have 4 different places where we have the logic for
modifying LED state of the various types of Ambient LEDs,
ExternalNotificationModule::SetExternalOff
ExternalNotificationModule::SetExternalOn
AmbientLightingThread::setLighting
AmbientLightingThread::setLightingOff

This patch de-duplicates the methods in ExternalNotification to
a single method, using a boolean to toggle whether we're turning
things on or off.
@fifieldt fifieldt force-pushed the external-notification-cleanup branch from 3825866 to 25de8e9 Compare October 29, 2024 09:58
@fifieldt
Copy link
Contributor Author

Thanks, I appreciate the guidance. Shall we merge this one as a start - since it'll be a while before I can get to the next steps?

@thebentern thebentern merged commit 2945b9c into meshtastic:master Oct 29, 2024
47 checks passed
caveman99 pushed a commit that referenced this pull request Nov 3, 2024
We currently have 4 different places where we have the logic for
modifying LED state of the various types of Ambient LEDs,
ExternalNotificationModule::SetExternalOff
ExternalNotificationModule::SetExternalOn
AmbientLightingThread::setLighting
AmbientLightingThread::setLightingOff

This patch de-duplicates the methods in ExternalNotification to
a single method, using a boolean to toggle whether we're turning
things on or off.
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