Solarbank1 schedule control and telemetry

[smariacher]

Adds telemetry and schedule control to Anker Solarbank (1) E1600

[flip-dots]

This is not bad, I have made a few changes, added it to the docs, added it to the API, and fixed some type issues. It would be good to have some tests added, like for testing the parsing of values, especially with how complex the schedule API is.

There are a few changes to the interface I would like to be made to make the library easier to use for others (like charging status, and the schedule), but for the most part its good.

Im not going to require it, since I dont have any existing tests for you to base it off, but it would be nice to also have a test or two for sending the schedule commands due to the complexity of it.

Writing tests is kind of a PITA, but its only so I don't break your code in future when inevitably end up refactoring stuff. Let me know if you need any help :)

[smariacher]

Sorry for the long wait, I hopefully have all the things you mentioned fixxed and/or changed. Now the only thing which I am unsure about is where to put the new ChargingSchedule dataclass I created. I don't know if there is any other device that does it like the SB1, so putting ChargingSchedule into the solarbank1.py file seems like the best option to me, but you may disagree. Maybe there is another better option I don't know of.

Here is the dataclass, maybe you have some comments here too. I implemented it in both the current_schedule property and the set_schedule function.

@dataclass
class ChargingSchedule:
    start_time: int
    """
    Start of schedule in minutes since midnight.
    """
    end_time: int
    """
    End of schedule in minutes since midnight.
    """
    output_wattage: int

    max_soc : int
    """
    Maximum SOC before Solarbank (presumably) goes into passthrough mode.
    """

    def __str__(self) -> str:
        """Convert the integer minutes back to HH:MM format for a nice display"""
        start_time_str = f"{self.start_time // 60:02d}:{self.start_time % 60:02d}"
        end_time_str = f"{self.end_time // 60:02d}:{self.end_time % 60:02d}"
        
        return (
            f"Charging Schedule:\n"
            f"   Time:    {start_time_str} - {end_time_str}\n"
            f"   Wattage: {self.output_wattage}W\n"
            f"   Max SOC: {self.max_soc}%"
        )

    def __post_init__(self):
        MIN_WATTAGE, MAX_WATTAGE = 0, 800 
        MIN_SOC, MAX_SOC = 0, 100
        
        if not (MIN_WATTAGE <= self.output_wattage <= MAX_WATTAGE):
            raise ValueError(
                f"Invalid output_wattage: {self.output_wattage}. "
                f"Must be between {MIN_WATTAGE} and {MAX_WATTAGE}."
            )
        
        if not (MIN_SOC <= self.max_soc <= MAX_SOC):
            raise ValueError(
                f"Invalid max_soc: {self.max_soc}. "
                f"Must be between {MIN_SOC} and {MAX_SOC}."
            )
        
        if not (self.end_time - self.start_time > 0):
            raise ValueError(
                f"Invalid time frame: Start: {self.start_time}, End: {self.end_time}. "
                f"Start time must be smaller than end time."
            )
        
        if not (self.start_time >= 0 and self.start_time <= 1440):
            raise ValueError(
                f"Invalid start time: {self.start_time}. "
                f"Start time cannot be less than 0 minutes or greater than 1440 minutes (24 hours)"
            )
        
        if not (self.end_time >= 0 and self.end_time <= 1440):
            raise ValueError(
                f"Invalid start time: {self.end_time}. "
                f"End time cannot be less than 0 minutes or greater than 1440 minutes (24 hours)"
            )

    @classmethod
    def from_time_strings(cls, start: str, end: str, output_wattage: int, max_soc: int) -> "ChargingSchedule":
        """Alternative constructor to create a schedule using HH:MM string formats."""
        return cls(
            start_time=cls.time_from_string(start),
            end_time=cls.time_from_string(end),
            output_wattage=output_wattage,
            max_soc=max_soc
        )

    @staticmethod
    def time_from_string(time: str) -> int:
        """
        Converts a string time in 24-hour HH:MM format to minutes since midnight.

        :param time: Time string in 24-hour HH:MM format.
        :returns: Minutes since midnight.
        """

        hours_str, minutes_str = time.split(":")
        hours = int(hours_str)
        minutes = int(minutes_str)
        
        if hours > 24:
            raise ValueError(f"Invalid hour value: {hours}. Hour must be between 0 and 24.")
        
        if minutes > 59:
            raise ValueError(f"Invalid minute value: {minutes}. Minute must be between 0 and 59.")
        
        if hours == 24 and minutes != 0:
            raise ValueError(f"Invalid time string: {time}. If hour is set to 24 then minutes may only be 0.")

        return hours * 60 + minutes

[flip-dots]

@smariacher its fine, I dont have the time to do a proper review until at least the weekend (but possibly later than that) due to upcoming coursework, but just taking a quick glance it looks good.

I think its fine to leave ChargingSchedule in solarbank1.py for now, but if it ends up being identical to other models I will probably end up moving it to states.py.

[thomluther]

I think its fine to leave ChargingSchedule in solarbank1.py for now, but if it ends up being identical to other models I will probably end up moving it to states.py.

SB1 schedule format is definitely different to SB Gen 2 and later.
SB Gen 2 and later also have different plans, with different structures and Gen 3 introduced more plans, again with different structure. So it will be better to keep it with the device class I guess.

[flip-dots]

SB Gen 2 and later also have different plans, with different structures and Gen 3 introduced more plans, again with different structure. So it will be better to keep it with the device class I guess.

Is it just the bytes that are different or is the abstract structure of starting at A time, ending at B time with output power C, and max SoC D different for others as well?

[thomluther]

SB Gen 2 and later also have different plans, with different structures and Gen 3 introduced more plans, again with different structure. So it will be better to keep it with the device class I guess.

Is it just the bytes that are different or is the abstract structure of starting at A time, ending at B time with output power C, and max SoC D different for others as well?

Different content.
From SB2 on, the normal time schedule slots only have a power value, not other values that SB1 still had per slot.
But SB2 can define different slot lists per weekday, which the SB1 did not support.

And SB2 and later comes with other plans with other structures:

• Time of Use plan
• Time Slot plan for dynamic tarifs
• A backup charge schedule (start and end type and a switch)^

To make it even more complex, what I have seen in MQTT commands is that adjusting the schedules uses the same command message and same command fields, but completely different structures for the fields, depending on the plan type contained in the command.

I have no idea where those plans end up in the telemetry data. Could be that query of the plan requires another specific command that is part of the normal telemetry data.
I did not look further into plan extraction or modification through MQTT, since that is too complex and lots of guesswork.
The can be changed easily through the Api and the cloud will send the required MQTT commands to the device.
That of course does not work with BT connection only.

[flip-dots]

Its looking much better now, I think it mostly just needs some tests so I don't end up breaking it in future as I will probably end up having to move some stuff around.

[smariacher]

Quick update since I haven't posted much for a while: I changed the code according to your feedback and wanted to test things one last time before commiting, but right now the weather is so bad that my PV modules don't produce any power making testing (e.g. charging/discharging status, (rapid) schedule changing etc.)

I would also like to submit some info about the quirks the SB1 brings with it. Stuff like minimum wattage output when using BLE vs cloud, battery output when solar input dips below the currently set schedule, wake up time for the inverters and so on. Where exactly would you like me to put this information @flip-dots?

[flip-dots]

@smariacher sounds good. The best place for documenting quirks would be the docs/source/solarbank1.rst file.

[smariacher]

This should now work schedule-setting wise, although there is still a weird quirk inside the schedule command I couldn't figure out: There is an option inside the app that controls if the SB1 should ignore PV input entirely and only output from the battery. This is reported back by setting the FamilyLoadSchedule's max_soc to 336. I am not sure if that's just my wrong parsing or if its really just using max_soc and a magic number to change the mode into battery out only.

Since there is a lot of work coming up for me I won't add much in the near future, I hope this is somewhat comprehensive for now. Sorry for the long wait and thanks again for everything @flip-dots!

[flip-dots]

@smariacher don't worry about the long wait, I totally get it, I also have poor availability at the moment as my dissertation and final exams are due imminently, so I can't really say when I am going to be able to fully review this, probably make some changes, get it merged, and eventually added to the Home Assistant integration, I plan on doing it when I have the time but that could be as long as 2 months away, I hope thats ok.

[flip-dots]

It would be good to have some tests for parsing the schedule from bytes from a device and for sending a new schedule to a device. A test was added in #38 which tests that the right bytes are sent when executing a command which could help, the exact test can be found here, don’t worry about making it pretty or its location, the tests are in desperate need of refactoring which I hope to start on soon so for now I just want to make sure there are enough tests to make it difficult for me to silently break anything in my refactoring.

[flip-dots]

Apologies for the wait but my availability should be a lot better now that my exams are sorted, this PR is in pretty great shape and it should be good to merge once the conflicts and issues are addressed.

[smariacher]

Hey flip-dots, hope your exams have gone well. This should now hopefully be ready to merge. If there is anything else, I am happy to work on that as well, just that I unfortunately wont be able to work on it from mid July to mid August.

[flip-dots]

Hey flip-dots, hope your exams have gone well. This should now hopefully be ready to merge. If there is anything else, I am happy to work on that as well, just that I unfortunately wont be able to work on it from mid July to mid August.

GitHub is still showing merge conflicts, can you fix those first please. I may or may not get around reviewing it today, but if not I will probably look tomorrow.

[smariacher]

I think I messed something up here, but I have no idea what exactly. Sorry for that, but could you maybe tell me what happened?

[flip-dots]

I think I messed something up here, but I have no idea what exactly. Sorry for that, but could you maybe tell me what happened?

No worries, merge conflicts are always a pain in the ass, I think I have fixed it and have done a quick read through and it passes a basic sanity check (and the tests) but if you could double check that everything is right and I haven’t accidentally removed anything important that would be great.

I really need to get around to fixing up the tests, using test parameterisation is great for reducing duplicate code but it also causes these god awful merge conflicts, I need to find a solution to that.

Anyway, so I am reasonably happy with this code now but I noticed that there is not a test for the values of the device, that the last thing before I am happy to merge this. The test_values tests exist to check that the correct values are reported by the functions (power out, solar, etc) for a given data packet, I usually use the demo.py script and then copy the packets and values into a text file which I then use to make the test if that helps (also sorry if I accidentally removed them when fixing the merge conflict). If you need any help with it let me know.

[jul1an-s]

@smariacher from what I've seen your FamilyLoadSchedule should be suitable for the SB2 as well. @thomluther is right that the SB2 supports additional, more complex schedule layouts, but the SB1 style works as well and is sufficient for my use case.
I'd rather put it in Solarbank common and I'll happily use it in the SB2. I can move it for you if it's fine for you and @flip-dots

[flip-dots]

@smariacher from what I've seen your FamilyLoadSchedule should be suitable for the SB2 as well. @thomluther is right that the SB2 supports additional, more complex schedule layouts, but the SB1 style works as well and is sufficient for my use case. I'd rather put it in Solarbank common and I'll happily use it in the SB2. I can move it for you if it's fine for you and @flip-dots

If its a forwards compatible structure then I think it makes sense to put it in states.py or a generic solarbank class and then have other classes override its functions as needed for additional functionality.

[jul1an-s]

I made up my mind and would like to propose this approach:

We have so many pending major changes in this project right now that I'd rather get a few PRs with limited scope merged than have this single big PR that solves everything.
I'll leave my PR as is and only fix the issues pointed out by @flip-dots . Leave yours as is as well. I can create a PR to move the FamilyLoadSchedule to a common file after we have our PRs merged.