davidusb-geek / emhass

emhass: Energy Management for Home Assistant, is a Python module designed to optimize your home energy interfacing with Home Assistant.
MIT License
260 stars 51 forks source link

Utils tweak secret param handling #167

Closed GeoDerp closed 5 months ago

codecov[bot] commented 5 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (6fe81cf) 88.54% compared to head (c982486) 89.95%.

Files Patch % Lines
src/emhass/utils.py 82.35% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #167 +/- ## ========================================== + Coverage 88.54% 89.95% +1.40% ========================================== Files 6 6 Lines 1668 1642 -26 ========================================== Hits 1477 1477 + Misses 191 165 -26 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

davidusb-geek commented 5 months ago

Hi. Are these necessary to avoid breaking the add-on? I'm not very fond of all these try/except, I was thinking about other ways to come around this.

GeoDerp commented 5 months ago

These ones in particular, could get around easily by just setting a default value as the other option to get a statement (having only one get statement). It would just mean it won't try and take the value from the secrets file first. (May be an issue if someone is running add-on mode not in HA. (which I'm guessing doesn't happen anyway)) The other try statements on the lists would need to be looked into deeper for another way around if the list exists and what to replace with if else. Maybe a simple fix. When I get some free time I'll have a look.

davidusb-geek commented 5 months ago

Is this related to theses changes? https://community.home-assistant.io/t/emhass-an-energy-management-for-home-assistant/338126/1779?u=davidusb

davidusb-geek commented 5 months ago

Is this related to theses changes? https://community.home-assistant.io/t/emhass-an-energy-management-for-home-assistant/338126/1779?u=davidusb

Well this is more a missing key on that default dict

GeoDerp commented 5 months ago

Both solar.forcast and solcast should be broken. That was the main reason for both pull requests initially. Would have to check if that's related to that comment however.

GeoDerp commented 5 months ago

Closed due to #169 being merged. Could reopen if someone is wrong with #169.

davidusb-geek commented 5 months ago

I did had to revert back to this solution after today's crash...

GeoDerp commented 5 months ago

@davidusb-geek sorry to hear, what went wrong, anything I can fix?

davidusb-geek commented 5 months ago

This issue https://github.com/davidusb-geek/emhass-add-on/issues/74#issue-2117220193

davidusb-geek commented 5 months ago

So I went back with the option on this PR, tested the add-on properly and published a patch release. I'm just about to release a new patched version to finally solve #154 that wasn't solved before because an error of mine.

GeoDerp commented 5 months ago

This issue https://github.com/davidusb-geek/emhass-add-on/issues/74#issue-2117220193

Would you like me to see if I can work out the issue?

davidusb-geek commented 5 months ago

Well like we discussed I did found that code more proper. Unfourtunately it failed but in the rush I didn't debugged that code completely and just switched to this PR. It could be interesting to improve to that other more proper version. But we will need to test the add-on properly before release. I did tested the docer standalone before release but it wasn't sufficient. So finally are you able to properly test the add-on using these dev containers? I added this option to test an add-on-like option.json but while using the docker standalone mode which is much more easy and fast to compile and test: https://github.com/davidusb-geek/emhass/blob/0f5335ffb102ce57b34e90cded4e7a817f96bce3/src/emhass/web_server.py#L140

GeoDerp commented 5 months ago

Personally I have been using this method to get semi close: https://github.com/davidusb-geek/emhass/pull/147#issuecomment-1925578995

But also there is a docker file in my Dev branch that also tries to emulate the add-on environment. Not as easily to use however.

In terms of running a HA host with a local compiled version of the add-on. Unfortunately I have not worked that out.

GeoDerp commented 5 months ago

Looks like the issue is with def_start_timestep/def_end_timestep. It's almost like options are not overriding the conf somehow (always being [0,0]. I'm wondering if reverting the pr fixed the issue ?

GeoDerp commented 5 months ago

For example setting def_start_timestep manually before the return in Utils build_params, then calling it in Wev_server still give the output:

web_server INFO  [0, 0]

Utils Screenshot from 2024-02-05 12-44-22 Web Screenshot from 2024-02-05 12-44-09

GeoDerp commented 5 months ago

Edit: I have worked out the problem and its possibly sooooo dumb

        associations.append(['optim_conf', 'def_start_timestep', 'list_start_timesteps_of_each_deferrable_load', 'start_timesteps_of_each_deferrable_load'])
        associations.append(['optim_conf', 'def_end_timestep', 'list_end_timesteps_of_each_deferrable_load', 'end_timesteps_of_each_deferrable_load'])

was

        associations.append(['optim_conf', 'def_start_timestep', 'start_timesteps_of_each_deferrable_load', 'list_start_timesteps_of_each_deferrable_load'])
        associations.append(['optim_conf', 'def_end_timestep', 'end_timesteps_of_each_deferrable_load', 'list_end_timesteps_of_each_deferrable_load'])

Well on the positive side I upgraded the testing environment I am using (mentioned here: https://github.com/davidusb-geek/emhass/pull/147#issuecomment-1925578995). Fixed a bunch of bugs