Support For Energy in pyatmo #494
Conversation
* Update BNCS * feat: add centralized ventilation controller NLLF --------- Signed-off-by: Tobias Sauerwein <cgtobi@gmail.com>
…, introduces a sum energy and an helper to be used in homeassistant primarily
… mode, add some tests
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @tmenguy - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟡 Complexity: 4 issues found
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -621,55 +600,332 @@ def __init__(self, home: Home, module: ModuleT): | |||
| super().__init__(home, module) # type: ignore # mypy issue 4335 | |||
| self.historical_data: list[dict[str, Any]] | None = None | |||
| self.start_time: int | None = None | |||
| self.end_time: int | None = None | |||
There was a problem hiding this comment.
suggestion (code_refinement): Consider initializing 'end_time' in the base class for consistency
To maintain consistency and ensure that all mixins have a similar structure, consider initializing 'end_time' in the base class or a common mixin.
|
|
||
| @time_machine.travel(dt.datetime(2022, 2, 12, 7, 59, 49)) | ||
| @pytest.mark.asyncio | ||
| async def test_historical_data_retrieval(async_account): |
There was a problem hiding this comment.
suggestion (testing): Consider adding edge case tests for historical data retrieval.
It would be beneficial to include tests that cover scenarios such as empty data returns, partial data availability, and data outside the expected range to ensure robustness.
|
|
||
|
|
||
|
|
||
| async def test_historical_data_retrieval_multi(async_account_multi): |
There was a problem hiding this comment.
suggestion (testing): Add validation for handling of API errors in historical data retrieval tests.
To ensure the resilience of the historical data retrieval, tests should also verify the correct handling of API errors, such as rate limits exceeded or server errors.
|
|
||
|
|
||
|
|
||
| async def test_historical_data_retrieval_multi_2(async_account_multi): |
There was a problem hiding this comment.
suggestion (testing): Include tests for data consistency over multiple retrieval calls.
To ensure data consistency, consider adding tests that perform multiple historical data retrieval calls and verify that the data remains consistent across these calls.
src/pyatmo/account.py
Outdated
| @@ -49,17 +57,43 @@ def __repr__(self) -> str: | |||
| f"{self.__class__.__name__}(user={self.user}, home_ids={self.homes.keys()}" | |||
| ) | |||
|
|
|||
| def update_supported_homes(self, support_only_homes: list | None = None): | |||
There was a problem hiding this comment.
issue (complexity): Consider simplifying the management of homes to reduce complexity.
The new implementation introduces a significant increase in complexity and code length, from handling additional data structures like all_account_homes, additional_public_homes, and support_only_homes, to incorporating more conditional logic and increased method responsibilities. This not only makes the code harder to follow but also increases the cognitive load required to understand and maintain it.
One way to address this complexity is by encapsulating the management of different types of homes into a separate class or structure. This could abstract away the intricate details of how homes are managed, stored, and accessed, making the AsyncAccount class more focused and easier to understand. For example, a HomesManager class could be responsible for all operations related to homes, such as updating their topology, finding specific homes, and managing the state of supported, public, and additional homes. This approach would help in keeping the code modular, making it easier to maintain and extend in the future.
| @@ -89,6 +89,42 @@ async def async_post_api_request( | |||
| timeout=timeout, | |||
| ) | |||
|
|
|||
| async def async_get_api_request( | |||
There was a problem hiding this comment.
issue (complexity): Consider consolidating request handling to reduce duplication and simplify the codebase.
The introduction of separate methods for handling GET requests (async_get_api_request and async_get_request) alongside the existing POST request methods adds complexity and duplicates logic with slight variations. This increases the cognitive load for developers trying to understand the error processing flow, especially with the added branches in the handle_error_response method for specific error codes. Additionally, the prepare_request_get_arguments method, which essentially returns its input without modification, introduces unnecessary indirection.
Consider consolidating request handling into a single method that can accommodate both GET and POST requests by specifying the HTTP method as an argument. This would reduce duplication and simplify the codebase. Simplifying error handling by consolidating logic into fewer branches could also make the code easier to follow and maintain. This approach aims to streamline the code and improve maintainability without sacrificing functionality.
| @@ -40,9 +40,12 @@ class Home: | |||
| name: str | |||
| rooms: dict[str, Room] | |||
| modules: dict[str, Module] | |||
| schedules: dict[str, Schedule] | |||
| schedules: dict[str, ThermSchedule] # for compatibility should diseappear | |||
| all_schedules: dict[dict[str, str, Schedule]] | {} | |||
There was a problem hiding this comment.
issue (complexity): Consider abstracting the schedule handling into separate classes for each schedule type.
The introduction of multiple schedule types and the handling of energy schedules significantly increases the complexity of the Home class. While these features are essential, the current implementation with nested dictionaries and extensive conditional logic makes the code harder to maintain and understand.
Consider abstracting the schedule handling into separate classes for each schedule type, inheriting from a common base. This approach would encapsulate the specific logic for handling different types of schedules (e.g., ThermSchedule, ElectricitySchedule) and reduce the complexity within the Home class itself. By doing so, you can simplify the access and management of schedule-related data and make the overall codebase more modular and easier to extend in the future.
Here's a brief example of how you might start to structure these classes:
class BaseSchedule:
def __init__(self, home, raw_data):
self.home = home
self.raw_data = raw_data
# Common schedule methods here
class ThermSchedule(BaseSchedule):
# Therm-specific logic here
pass
class ElectricitySchedule(BaseSchedule):
# Electricity-specific logic here
pass
class Home:
def __init__(self, auth, raw_data):
self.auth = auth
self.entity_id = raw_data["id"]
self.name = raw_data.get("name", "Unknown")
self.schedules = self._handle_schedules(raw_data.get(SCHEDULES, []))
def _handle_schedules(self, raw_data):
schedules = {}
for s in raw_data:
if s['type'] == 'therm':
sched = ThermSchedule(self, s)
elif s['type'] == 'electricity':
sched = ElectricitySchedule(self, s)
else:
continue # Handle unknown schedule type
schedules[s["id"]] = sched
return schedulesThis snippet demonstrates how you might begin to refactor the schedule handling into a more manageable and scalable structure.
| self.timetable = [ | ||
| TimetableEntry(home, r) for r in raw_data.get("timetable", []) | ||
| ] | ||
| self.zones = [Zone(home, r) for r in raw_data.get("zones", [])] | ||
|
|
||
|
|
||
| @dataclass |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the class structure to reduce complexity.
The introduction of several new classes and the use of inheritance to create specialized schedules significantly increases the complexity of the codebase. While the added functionality necessitates some complexity, there are strategies that could simplify the implementation and make the code easier to maintain:
-
Consider Composition Over Inheritance: If the new schedule types share common functionality, using composition instead of a deep inheritance hierarchy could reduce complexity. This involves defining common behavior in utility classes or functions that are utilized by the schedule classes.
-
Evaluate the Necessity of All Classes: Simplify the class structure by assessing whether all the newly introduced classes are essential, or if some can be combined or simplified. This is particularly relevant if the difference between certain classes (e.g.,
ThermScheduleandCoolingSchedule) is minimal. -
Reduce Boilerplate Code: For very similar schedule types, a more dynamic approach could help minimize boilerplate code. Common attributes across schedules could be set in the base class or through a utility function, reducing redundancy.
Here's a suggestion to use a schedule_specifics dictionary for storing type-specific attributes within a single Schedule class, rather than having multiple specialized classes. This approach aims to balance flexibility with simplicity, making the codebase more manageable:
@dataclass
class Schedule(NetatmoBase):
selected: bool
default: bool
type: str
timetable: list[TimetableEntry]
zones: list[Zone]
schedule_specifics: dict # Use a dictionary to store type-specific attributes
def __init__(self, home: Home, raw_data: RawData) -> None:
super().__init__(raw_data)
self.home = home
self.type = raw_data.get("type", "therm")
self.selected = raw_data.get("selected", False)
self.default = raw_data.get("default", False)
self.timetable = [TimetableEntry(home, r) for r in raw_data.get("timetable", [])]
self.zones = [Zone(home, r) for r in raw_data.get("zones", [])]
self.schedule_specifics = self._parse_specifics(raw_data)
def _parse_specifics(self, raw_data: RawData) -> dict:
specifics = {}
if self.type in ["therm", "cooling"]:
specifics["away_temp"] = raw_data.get("away_temp")
specifics["hg_temp"] = raw_data.get("hg_temp")
if self.type == "cooling":
specifics["cooling_away_temp"] = raw_data.get("cooling_away_temp", specifics.get("away_temp"))
return specificsThis simplification could make the codebase more approachable for new developers and reduce the cognitive load when making future modifications or additions.
There was a problem hiding this comment.
Thank you for your effort. I feel that this makes the code quite hard to follow and I am not sure if this solves the energy tracking properly. Netatmo has been quite sensitive to aggressive polling and I don't think we actually need this. No matter if you fetch data ever minute or every 10 seconds, it will be far off from realtime. Please correct me if I misinterpret your intention. Also I think this PR tries to solve too many things at once, which makes reviewing quite difficult.
src/pyatmo/account.py
Outdated
| self.support_only_homes = support_only_homes | ||
| self.all_account_homes: dict[str, Home] = {} | ||
| self.additional_public_homes: dict[str, Home] = {} |
There was a problem hiding this comment.
What do these mean, can you please elaborate?
There was a problem hiding this comment.
hi, first thx for review: I have added the notion of "selected homes" but wanted to keep everything the same vs the previous implementation, hence keeping in separate bucket the different types of homes we now have.
src/pyatmo/account.py
Outdated
| @@ -49,17 +57,43 @@ def __repr__(self) -> str: | |||
| f"{self.__class__.__name__}(user={self.user}, home_ids={self.homes.keys()}" | |||
| ) | |||
|
|
|||
| def update_supported_homes(self, support_only_homes: list | None = None): | |||
There was a problem hiding this comment.
What is support_only_homes?
There was a problem hiding this comment.
see above : as set of homes that you define in your option config (coming from HA part) : see the readme of the Netatmo HA component I tried to explain :) this variable is for the list of homes the user decided to "import" or support in his HA instance, and hence I reflect that in the account itself, handy to target only a set of homes, all, or only one. Ex : if you have a main home and a secondary vacation house, you probably don't want to have your vacation house in your main house HA instance
| if not self.home.energy_endpoints: | ||
| return 1 | ||
| else: | ||
| return len(self.home.energy_endpoints) | ||
|
|
||
| async def async_update_measures( |
There was a problem hiding this comment.
This function is way to big and deeply nested. If it actually needs to do all that this needs to be cut into manageable and testable functions.
There was a problem hiding this comment.
are you talking about async_update_measures?
There was a problem hiding this comment.
ok it is big, will refactor a bit, but it will be artificial to break in small pieces ... I personally prefer reading everything at once :)
src/pyatmo/modules/module.py
Outdated
| self.sum_energy_elec_peak = 0 | ||
| self.sum_energy_elec_off_peak = 0 | ||
|
|
||
| def get_sum_energy_elec_power_adapted(self, to_ts: int | float | None = None, conservative: bool = False): |
There was a problem hiding this comment.
This code is too nested, which makes it hard to read, hard to test and error prone.
There was a problem hiding this comment.
are you talking about get_sum_energy_elec_power_adapted ?
There was a problem hiding this comment.
I'll refactor a bit (even if this one is honestly not so big)
Hi ... yes I do think you missed some stuff here :) and they are in the home assistant component: All in all yes the code is more complexe but: I have a massive installation of Legrand devices (90 measure points) and it is working quite well, no throttling from the API (but well yes some timeout but it is different) Hope it helps to understand the work here :) BR |
|
Small nudge :) |
There was a problem hiding this comment.
Hi @tmenguy, front up, I just want to say sorry that it took me so long to get back to this.
What bugs me about this huge PR is that it mixes things that in my opinion could be separated into multiple PRs. For example the support for only processing a selection of homes available to that account would be significant enough feature to be reviewed as a whole and this should be done as non-breaking as possible to be backwards compatible with the current HA integration and possible other uses as well. The same goes for the scheduling part. It is also a feature of its own and not tied to the others, no? So IMO this should be three separate PRs which would also make it easier to be non breaking and smaller to review.
Hi,
This one is the companion of https://github.com/tmenguy/netatmo_custom.
All of this is to add energy measure support in homeassitant, so for pyatmo, extended handling of the /getmeasure endpoint. This one is not really up to date by the hour, from what I've seen it is updated very 3 to 4 hours, so I introduced an estimation using power and the measure to have a proper following of energy in homeassitant that is made to be "close to real time" .
Of course feel free to ask questions and so on. What I don't know is : is this one is working with the existing home assistant integration, it should, it is fixing some issues, but well, we never know, and I don't have a full HA setup for dev, more one for HACS.
BR
Thomas