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 light specs for yeelight #1163

Merged
merged 11 commits into from
Oct 22, 2021
Merged

Conversation

Kirmas
Copy link
Contributor

@Kirmas Kirmas commented Oct 20, 2021

This is base support for spec file & uses it to provide information about supported models. Second PR based on #1094 (review)

@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2021

Codecov Report

Merging #1163 (83172cc) into master (9c04559) will increase coverage by 1.28%.
The diff coverage is 95.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1163      +/-   ##
==========================================
+ Coverage   76.61%   77.90%   +1.28%     
==========================================
  Files          77       79       +2     
  Lines        9155     9222      +67     
  Branches      756      762       +6     
==========================================
+ Hits         7014     7184     +170     
+ Misses       1958     1853     -105     
- Partials      183      185       +2     
Impacted Files Coverage Δ
miio/integrations/yeelight/__init__.py 87.94% <88.88%> (-0.08%) ⬇️
miio/integrations/yeelight/spec_helper.py 94.87% <94.87%> (ø)
...ations/yeelight/tests/test_yeelight_spec_helper.py 100.00% <100.00%> (ø)
miio/__init__.py 100.00% <0.00%> (ø)
miio/g1vacuum.py 74.14% <0.00%> (+74.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c04559...83172cc. Read the comment docs.

miio/integrations/yeelight/__init__.py Outdated Show resolved Hide resolved
miio/integrations/yeelight/spec_hellper.py Outdated Show resolved Hide resolved
miio/integrations/yeelight/spec_hellper.py Outdated Show resolved Hide resolved
miio/integrations/yeelight/spec_hellper.py Outdated Show resolved Hide resolved
miio/integrations/yeelight/spec_hellper.py Outdated Show resolved Hide resolved
miio/integrations/yeelight/spec_hellper.py Outdated Show resolved Hide resolved
@rytilahti rytilahti changed the title Add specs file for yeelight / _supported_models now use specs.yaml Add light specs for yeelight Oct 20, 2021
@Kirmas
Copy link
Contributor Author

Kirmas commented Oct 20, 2021

  1. Add light specs for yeelight #1163 (comment) and Add light specs for yeelight #1163 (comment) maybe this is related to my previus behaver (c++). But as i understand this looks like memory over budget. Ideally we need this data only during devises initialization and after this Yeelight(Device) can save related spec data and forget about other. Please correct me.

  2. Add light specs for yeelight #1163 (comment) and @DataClass is unknown in the python 3.6 (I really bad with python class attributes. )

@rytilahti Could you give some advice with 1 and 2

@rytilahti
Copy link
Owner

But as i understand this looks like memory over budget. Ideally we need this data only during devises initialization and after this Yeelight(Device) can save related spec data and forget about other. Please correct me.

So, we could only initialize it only once inside the module. Think about a situation with 10 yeelight bulbs, calling the static method would cause file I/O & parsing for each and every instance. This would also skip those if the Yeelight module never gets initialized. Here's what I think could work:

class Yeelight(Device):
    _supported_models = None  # ignoring now the fact that we may want to have this statically defined in the future, for autodetection..
    _spec_helper = None

    def __init__(self, ...):
        if Yeelight._spec_helper is None:
            Yeelight._spec_helper = YeelightSpecHelper("file")
            Yeelight._supported_models = Yeelight._spec_helper.supported_models()

        self._model_info = Yeelight.get_model_info(self.model)

    @property
    def color_temperature_range() -> ColorTempRange:
        return self._model_info.color_temp

Does that make sense to you?

2. @DataClass is unknown in the python 3.6 (I really bad with python class attributes. )

Better use @attr.s(auto_attribs=True) like done in https://github.com/rytilahti/python-miio/blob/master/miio/gateway/devices/subdevice.py#L17 for now then, I think.

@Kirmas
Copy link
Contributor Author

Kirmas commented Oct 21, 2021

So, we could only initialize it only once inside the module. Think about a situation with 10 yeelight bulbs, calling the static method would cause file I/O & parsing for each and every instance. This would also skip those if the Yeelight module never gets initialized. Here's what I think could work:

I`m not satisfied but agree I/O for home assistant(or any other CD card user) more important than RAM. Hope in the future SSD will be cost nothing and all off us start to use it (and size must be like CD card) :) @rytilahti Please look current state.

Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

Just a couple of small changes and this is good to go.

miio/integrations/yeelight/spec_helper.py Outdated Show resolved Hide resolved
miio/integrations/yeelight/spec_helper.py Outdated Show resolved Hide resolved
miio/integrations/yeelight/spec_helper.py Outdated Show resolved Hide resolved
value["background_light"],
value["supports_color"],
)
YeelightSpecHelper._models[key] = info
Copy link
Owner

Choose a reason for hiding this comment

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

Even when improbable that someone adds a duplicate entry to the file, I'd check here if the key already exists and raise an exception if so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this but as I understand yaml.safe_load ignore key duplicates. I tried to add duplicate and wouldnt catch exception :(

Copy link
Owner

@rytilahti rytilahti Oct 22, 2021

Choose a reason for hiding this comment

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

Okay, it's a dictionary already so new values will override the existing one. Let's do the following: remove the duplicate handling (no need to check for the key & raise the exception) and let's merge this then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This-Russian-artists-comics-will-make-you-win-your-day-60338d74658b9__700

Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

Thanks @Kirmas! 🎉

@rytilahti rytilahti merged commit e1adea5 into rytilahti:master Oct 22, 2021
@Kirmas Kirmas deleted the yeelight_addspecs branch October 22, 2021 18:54
@rytilahti rytilahti mentioned this pull request Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants