danielwippermann / resol-vbus-java

A Java library for processing RESOL VBus data
7 stars 7 forks source link

Add Regumaq X-45 ConfigurationOptimizer #25

Closed karolinschlegel closed 2 years ago

karolinschlegel commented 2 years ago

Hello,

I have created a ConfigurationOptimizer for the Regumaq X-45 from Oventrop. All parameters are listed, but the parameters which I haven't used are commented out, since no optimizing is implemented yet.

danielwippermann commented 2 years ago

Hi @karolinschlegel !

Thank you for the pull request and sorry for the late reply.

I have three questions:

1) Would it break much of your code if I rename the configuration optimizer to include the vendor name? With OventropRegumaqX45ConfigurationOptimizer it would adhere to the naming convention I used on other optimizers and libraries in the past?

2) Have you already used the generateClockConfiguration method on the optimizer? I don't really know whether it would work because the value referenced there should be something different. If you do not neet it at the moment, can I throw a "not implemented yet" exception there until somebody needs it?

3) Would you mind being listed as contributor in the README?

Thanks again, Daniel

karolinschlegel commented 2 years ago

Hi @danielwippermann and thank you for checking my PR.

Would it break much of your code if I rename the configuration optimizer to include the vendor name? With OventropRegumaqX45ConfigurationOptimizer it would adhere to the naming convention I used on other optimizers and libraries in the past?

No, that's no problem at all.

Have you already used the generateClockConfiguration method on the optimizer? I don't really know whether it would work because the value referenced there should be something different. If you do not neet it at the moment, can I throw a "not implemented yet" exception there until somebody needs it?

I'll fix that. That's a copy-past-error.

Would you mind being listed as contributor in the README?

I would be happy!

I have another question: Would it be beneficial for the OpenHAB binding where I'm currently working on, if the ConfigurationValue class would contain the type (Root type from the table) so that consequently a conversion method could be implemented? For instance:

ConfigurationValue warmwasserTSollRaw = ConfigurationValue("WarmwasserTSoll", 0x0037, 0x5fc5f901, 450, 0, false, false);
RosApp warmwasserTSoll = RosApp.convert(warmwasserTemp);

where warmwasserTSoll contains a RosApp:TemperatureShort set to 45,0 Do any code transformation tools exist to bring the RosApp types into resol-vbus-java?

danielwippermann commented 2 years ago

Hi @karolinschlegel! Thanks for the quick response and code changes! Good job!

Do any code transformation tools exist to bring the RosApp types into resol-vbus-java?

No, such a tool does not yet exist, but I'll have a look whether I can derive something from the underlying ROS information. I'll get back to you if / when I made progress.

karolinschlegel commented 2 years ago

Thanks for the quick response and code changes! Good job!

Thank you for maintaining the library.

No, such a tool does not yet exist, but I'll have a look whether I can derive something from the underlying ROS information. I'll get back to you if / when I made progress.

Thanks, I'll look forward to it. In the meantime I will prototype the integration of the types in OpenHAB. As soon as the RosApp types are available, I will change the implementation in the add-on to them.

ramack commented 2 years ago

@karolinschlegel great to read that you plan to work on vbus-java openHAB improvements! What are your plans?

karolinschlegel commented 2 years ago

Hi @ramack, I'm adding the functionality to write parameters in the controllers to the plugin. I will publish a pull-request soon™.

ramack commented 2 years ago

That's great but keep in mind, that the parameters in the resol products are not intended to be written too often as they are directly written into flash memory (@danielwippermann correct me if I'm wrong) and with OpenHAB it could be easy to write these values every second and we might need to protect users doing so in the binding to not damage their devices - they are not developers, knowing what they do.

karolinschlegel commented 2 years ago

Hi @ramack, thanks for pointing that out. I already read through the discussion in https://github.com/danielwippermann/resol-vbus-java/issues/15#issuecomment-390469856 where @danielwippermann explained that. Indeed, that's a common design issue with control systems. I was hoping, that there may be a distinction between saveConfiguration and setConfiguration. I would have interpreted that saveConfiguration writes to nv memory while setConfiguration writes to volatile memory and thereby only changing the runtime configuration. Maybe you could clarify that, @danielwippermann? If not, the implementation will get more sophisticated, since some kind of cache would be necessary as a workaround, as well as an expert only switch with a disclaimer, protecting said users.

ramack commented 2 years ago

or maybe a simple counter to reject update if they are too frequent (and some documentation about the issue)

danielwippermann commented 2 years ago

I was hoping, that there may be a distinction between saveConfiguration and setConfiguration. I would have interpreted that saveConfiguration writes to nv memory while setConfiguration writes to volatile memory and thereby only changing the runtime configuration. Maybe you could clarify that, @danielwippermann?

That was the original intention of having two command, but as far as I know none of the current controllers do this differentiation any more and instead go through the same code path. And there is no other, explicit way to influence the storage behaviour from outside the controller.

There are some protections implemented in the controller. So if you really manage to change a persistent value once per second it won't be stored at all because every set operation should postpone the next write to "in five seconds". But if you change the value outside of the "five second" window (e.g. once every 10 seconds), you will wear out the NVM chip quite quickly.

If not, the implementation will get more sophisticated, since some kind of cache would be necessary as a workaround, as well as an expert only switch with a disclaimer, protecting said users.

What I have done in a different project (and programming language) was a cache that wrapped the ConfigurationOptimizer, exposing the same set of methods, but adding two higher-level features:

I assume that @karolinschlegel has something similar in mind.

danielwippermann commented 2 years ago

There are some protections implemented in the controller. So if you really manage to change a persistent value once per second it won't be stored at all because every set operation should postpone the next write to "in five seconds". But if you change the value outside of the "five second" window (e.g. once every 10 seconds), you will wear out the NVM chip quite quickly.

And I would not rely on the presence or configuration of that feature alone because it might be changed in the future of course.

karolinschlegel commented 2 years ago

That was the original intention of having two command, but as far as I know none of the current controllers do this differentiation any more and instead go through the same code path. And there is no other, explicit way to influence the storage behaviour from outside the controller.

Yes, it would be nice if that were incorporated into the firmware.

I assume that @karolinschlegel has something similar in mind.

Exactly :slightly_smiling_face: