Add OHPCF support as CEM client actor

[heavyweight87]

This OHPCF implementation currently implements read and write.
Events/updates we will support later as we need other use cases in order to check that it works

This was tested with a Vailant heatpump

The public api is quite basic but we can improve it once the events work

[DerAndereAndi]

Thanks @heavyweight87 for this great start! I will look into at as soon as possible.

Some quick thoughts:

• As a target the public APIs should not use SPINE data models, model.SmartEnergyManagementPsDataType is quite complex as well.
• If the remote device (heatpump) returns partial data (e.g. notify), the SPINE stack currently has to do a full read, as data merges are not yet supported. So the current state here is not ideal. The current merge doesn't work, as the data structure here is completely different

Looking forward to getting this included

[heavyweight87]

Hi,

• I agree there should be some kind of abstraction like you did with LoadLimit. Im not 100% what to add in this struct for now, maybe it will be clearer once the notifications are working (unless you have a suggestion?)
  Keo have some very strange abstractions for this usecase

• I understand that you are working on adding support for partial reads. I think we will come back to finishing notifications once the other usecases are implemented as we will need to set DHW and room temperatures in order to test it.

[DerAndereAndi]

I don't have a suggestion at the moment, sadly.

Also I am not working on partial reads, as I do not see a benefit at the moment. It makes things much more complicated. In addition: this would also require a merge for this data structure in SPINE to be available, which is not the case yet.

[heavyweight87]

Whats the idea behind partial reads? Is it just to save data transfers? Is it mandatory or just optimization?

I understand an issue may be also that it would be hard to tell which data has changed, but I guess there is an applicative solution to this.
Do you think these issues prevent us from merging these changes?

[DerAndereAndi]

Think of a function in a feature belonging to an entity to be a database table. This table can contain multiple entries, e.g. LPC and LPP in the same table, so a limit of allowed production or consumption. Now depending on the device, you may only care about consumption. So the code has to be able to fetch the corresponding data points from the table, and also only update the proper datapoints on changes.

To implement the CEM side, it is technically not mandatory, but for the actor compressor it is.

So from a stack perspective, it has to be implemented at some point, as full support for this usecase for all actors can not be implemented.

Now to know what the connected devices current data state is, the stack maintains a digital twin with all known data. So whenever it sends partial data and merge is not supported, it has to make a full read as a workaround.

In addition when providing events to the application, that are as specific as possible, one has to digest the incoming data and know what is changed. For that manual code has to be written anyway.

I can't say if these issues prevent anything, until I fully understand the implementation, the use case spec and see actual data communication to verify.

EEBUS is a monster and it is way too complex. When seeing the public APIs of implemented use cases, you won't recognize that. Only when going down the stack and see what is actually going on.

This is not the complete story, just a quick overview.

[heavyweight87]

Yes it seems extremely over engineered
I hope we arent starting something we can't finish then!

[DerAndereAndi]

I don't think there will be a showstopper. There will be just work to be done that is a bit more complex than the current code in the PR ;)

[DerAndereAndi]

Thanks for the implementation so far, well done!

Would you mind integrating the requested changes as well as fixing the linter warnings?

One of the main goals at this API level is to hide the SPINE complexity. Therefor NOT returning or passing a SPINE data structure is very important. I know this is complex in this scenario as the data structure is very complex. But the usage should be simplified a lot, therefor I'd love to see such a change being added.

[DerAndereAndi]

Right now the API tries to avoid returning SPINE data models to the application, because these models are way too complex especially in this case. Instead it would be great to provide a simplified data structure which contains only the data required by the application.

Would it be possible to for you to design such a data structure here?

[heavyweight87]

Agreed! Im not 100% sure how this structure should look like, and one of the issues is I cant fully use OHPCF until other use cases are completed (we need to be able to set temperature and configure the mode at least).

Once these other usecases are completed and I have a better idea of what the top level API should look like i'll update this.

[DerAndereAndi]

Could you please add tests for this file as well?

[DerAndereAndi]

This method is not part of the API interface. Was this missed? If so, I'd love to see some tests here as well. Additionally I would love use a different data structure instead of requiring the SPINE data model to be passed.

[DerAndereAndi]

Events for OHPCF data being updated should be added, similarly to how it is done in the OPEV implementation.

[heavyweight87]

Sure - once the other use cases are working to allow OHPCF to be triggered/activated ill add the events in. I guess OHPCF should have been the last use case to be implemented not the first!

[DerAndereAndi]

I changed this PR to draft mode. Feel free to change that again when you feel it being read for review.

[heavyweight87]

My colleague finished this usecase, he will open a MR when unit tests are added