Refactor TEDAPI __init__ for readability/code reuse

[mccahan]

This might be a bit much to bite off, so my feelings aren't hurt if we just skip it. I started just splitting the protobuf messages out to make the file more readable but then went on a yak-shaving expedition for a few things

Primary goal here is that the tedapi init file is massive and has a lot of copy/paste repeated code in it. To that end:

• Let the @uses_api_lock decorator handle acquiring the locks
• Add @uses_cache(key) and @uses_connection_required decorators so each function call doesn't have to repeat that code
• Add a unified __run_request function to handle throttling, exceptions, etc
• Split the protobuf messages into their own classes with encode/decode functions, because they're massive and made the init file so much more difficult to read
• Move the big chunks of the vitals dictionary to vitals_dictionary.py for readability

From my testing everything seems to be working, but I only have a single PW3 so might be missing hardware to thoroughly test some pieces.

@Nexarian this is probably pertinent to your interests

[jasonacox]

Thanks @mccahan !

Yes, I agree, it is a lot but I don't hate it. 😄 I pulled it down and the basic functions work, but pulling vitals throws errors:

File "/pypowerwall/tedapi/vitals_dictionary.py", line 221, in __get_pvs_and_pvac_strings
pvac_strings[pvac_name]["PVAC_PvState_A"] = "PV_Active" if string_a else "PV_Disabled"
~~~~~~~~~~~~^^^^^^^^^^^
KeyError: 'PVAC--1538100-00-F--CN************J'

I didn't unwind the new vitals code so could be something simple.

Some things I consider for refactors like this:

• Is it as simple as we can make it for maintainability?
• Is it performant for tiny systems (e.g. RPi or NAS installs)?
• Are there edge cases (Powerwall systems vary a lot) that would break?

[mccahan]

Theoretically, I may have fixed that issue in 5a7ce77 - apparently I don't have whatever kind of hardware that is? I tested this by opening /vitals on my production instance side-by-side with this branch and it matched, so I'm assuming I don't have that.

• Personally I think it's easier to maintain when you've got things a little more modularized - finding things in that big file was a little dizzying, and having cache/locks as decorators feels like it simplifies
• Should be zero impact on tiny systems - there's really no new logic happening that wasn't happening already, and compared to the latency of waiting for responses from the PW itself anything we do here is just rearranging deck chairs on the titanic

[Nexarian]

Overall: I like the direction this is going.

Functionality wise, my biggest concern is the change in how the api lock works. That is a big shift and I'm not yet convinced we want that.

Otherwise: I encourage you to break this pull request up into smaller increments. You need not move every single function over to your more modularized version at once. Do one at a time. That makes it easier to review and makes the blast radius from any bugs much smaller.

[mccahan]

You're not wrong this should've been two PRs - I did pull the vitals part out into a separate branch just now (https://github.com/jasonacox/pypowerwall/compare/main...mccahan:chore/refactor-tedapi-vitals?expand=1) though I'm not sure I want to complicate a merge on this branch by removing/rebasing later. I did the side-by-side compare of /vitals on mine and they match 1:1, though I just have the single PW3 (no Neurio, etc).

The biggest risks (actual changes) are:

• Locking and the throttling
• Vitals tweaks for any hardware that doesn't match what I have

[Nexarian]

Re: Pain of rebasing. In the case of these gigantic refactors, the fact that you're scared to do it is a signal it's too big.

In those cases I often don't rebase, but re-create from whole cloth using the old branch as a reference, that's often easier.

[mccahan]

Nah it's not fear of a rebase, it's that the fun part is over and the ADHD will kick in and I'll lose interest if I'm playing rebase tag lol. We could PR in https://github.com/jasonacox/pypowerwall/compare/main...mccahan:chore/refactor-tedapi-vitals?expand=1 as the smallest digestible chunk, and deal with that merge conflict if that merges first, but otherwise I'll probably decline trying to split this up function-by-function into a bunch of separate PRs. That feels like extra testing to me, dragging out a mystery piecemeal.

[Nexarian]

@mccahan, this is really up to @jasonacox at this point :)

ADHD: https://www.youtube.com/watch?v=Zvqx9DfG9lU

[jasonacox]

Sorry for the delay. Besides some personal emergencies, this is a lot to review. While I also like the work/direction, I have some concerns.

• From a code style perspecitive (yes, I'm guilty of not publishing anything to help) this is the first PR of the project using camel case for files. It feels off. Either we use it globally or we should reconsider the structure to match the existing.
• I've started worrying a bit more about the API locking mechanics. I think @Nexarian mentions this and did a good job in a recently PR to help us improve, but I see things like The Outer Limits: A Magical Time Period on Random Hours? #158 and wonder if we still have issues. Ccould be unrelated completely but still I'm concerned on how we test especialy in such a large PR.
• Said by both of you, this is a big change. We have done them before based on great work by others to refactor, but I can't help but wonder how we could decouple some of this change to ensure time to bake between changes. How would we do that?

I would appreciate thoughts and perspectives. I'm still going through the code but I do have a lot going on now. Thanks for your patience.

[Nexarian]

@mccahan Based on @jasonacox's comments: If your ADHD really does prevent you from splitting this up and staying the course, I'm happy to do it. I do this kind of thing all the time at my job, and I'm eventually going to have to do it with my refactor of the proxy.

But this is your change not mine, and I won't without your blessing.

[mccahan]

Zero rush at all, hope things work out okay.

I swapped the camel casing (habit, didn't even notice), but otherwise if this is too much I'm happy to just abandon this PR. It's essentially all copying/pasting around, so beyond theoretical maintainability there's no gain. I'd personally err on the side of bundling it rather than stringing it across a few versions so it's easier to pinpoint when something happened if there's a bug report.

My test scenario has been running it side-by-side with the current production version and checking the results for each function; the substance parts of it would fail on requests because of changes to the caching, or the vitals wouldn't match up side-by-side (N.B. I have just the PW3 with none of the Neurio stuff).

[Nexarian]

There is another way: Unit tests. Right now we have 0. We can cast the current behavior into unit tests, and if we have good enough coverage, refactor fearlessly :)

[mccahan]

@Nexarian be my guest! I pulled a chunk out of it into #159, but if you'd like to split this one up you're more than welcome to. I wouldn't split it too far so you don't end up sprawling it over six versions trying to trace down any issues, but whatever works there. I'm also not at all hurt if we ditch this entirely.

(You just commented live lol) the only shortcoming to unit tests for this piece is that it wouldn't catch the primary risk of this particular PR, which is around testing it with hardware that I don't have. I think your hardware setup is more complicated, so maybe a few collections of responses from different systems to unit test vitals responses against could catch it, though that might become tedious quick if they start changing firmware responses.

[Nexarian]

I wouldn't split it too far so you don't end up sprawling it over six versions trying to trace down any issues
That's exactly what I would do. If you keep each change smaller and more manageable and isolated to unique minor versions, it's really easy to find where things went astray and identify the root cause and fix quickly. Version number increments are cheap, bugs are not.

Alright, I'll work on this then. I think merging in the spirit of your changes before my crazy proxy refactor would do all of us a lot of good.