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

refactor/deprecate - hardware module #228

Open
JarbasAl opened this issue May 16, 2024 · 4 comments
Open

refactor/deprecate - hardware module #228

JarbasAl opened this issue May 16, 2024 · 4 comments
Labels
breaking enhancement New feature or request refactor code improvements with no functional changes

Comments

@JarbasAl
Copy link
Member

JarbasAl commented May 16, 2024

the hardware module was ported from the mycroft mark 2 in order to implement the mk2 enclosure, but it has some problems

  • unlike everything else in this package, this does not provide a plugin class to be used in a downstream entry point, it's just fully optional helper classes, conceptually I'm not sure they belong here or elsewhere
  • colors is a minimal implementation with hardcoded color values, it should use the color utils from lingua-franca, or at least we should not have 2 classes with same name in official ovos packages, makes code ambiguous and hard to import at same time
  • switches is not a abstract switch at all, it represents only the mk2 switch combination, this should be fully deprecated IMHO
  • fan is similar to switches, i could see an argument to keep it but dont feel the cpu_temperature doesnt really belong here, this should be fully deprecated IMHO
  • they all refer to "capabilties" which was a mark2 rewrite branch concept, it doesnt apply to ovos and that is dead code
  • they all ignore base event handlers from the PHAL base class, if we introduce methods for specific actions then those should be used in default event listeners from base class

i believe the LED class is useful since it is common across many plugins, so that one should be modernized and everything else dropped.

@JarbasAl JarbasAl added enhancement New feature or request refactor code improvements with no functional changes breaking labels May 16, 2024
@NeonDaniel
Copy link
Member

I'm not sure this should drag Lingua Franca and language parsing into a hardware interface; minimally I think the interface should include methods to accept and translate hex/hsl/rgb values to whatever the particular LED object wants.

I think the hard-coded values should be kept for primary/secondary colors and black/white only (red, orange, yellow, green, blue, violet, black, white). A plugin/skill could use LF externally to determine a hex/hsl/rgb value to pass to the LED object.

Switches and fan I am less worried about, but I do believe some abstraction is helpful so each plugin doesn't need to implement pull_up/pull_down setup and can just set config and get a binary "on"/"off" value to work with.

The fan I likewise think is helpful since there is common logic for how to manage fan speed based on CPU temp, but it may not be used in practice since ideally that would be controlled by the system.

@NeonDaniel
Copy link
Member

Releated LED animation issue #94

@builderjer
Copy link
Member

builderjer commented May 27, 2024

Right now, the animations are coded to use the Color class from OPM

def __init__(self, leds: AbstractLed, color: Color):

If this was loosened to use a tuple instead, plugins could use Lingua Franca for colors and still use these animations.

Edit:

This isn't quite right. There are several places where the colors from OPM are hard coded into the animations. It makes it impossible to use Lingua Franca for color choice and these animations at the same time.

@builderjer
Copy link
Member

Perhaps a separate ovos-PHAL-plugin-leds is in order? Then it can accommodate for several different types?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking enhancement New feature or request refactor code improvements with no functional changes
Projects
None yet
Development

No branches or pull requests

3 participants