[hdpowerview] Preparation for Generation 3 API #13352
Conversation
Signed-off-by: Andrew Fiddian-Green <[email protected]>
andrewfg
changed the title
andrewfg
added
the
enhancement
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
|
For testing purposes the Jar file is here.. |
andrewfg
added
rebuild
|
Partial review done with no comment. |
@andrewfg - I recently started looking into this also, continuing from #12678. I'm preparing some notes and also wanted to prepare some implementation propositions, but didn't get to this yet. Since you started the review, @lolodomo, I'll just for now share some general preliminary comments. First, the new API is actually a completely new API. It seems to me that it is more different than it is similar. Therefore my first suggestion would be to keep the DTO's completely separate. In the current approach they are entangled, which makes it harder to maintain. Bugs can be introduced in Gen 2 when making changes for Gen 3 and vice versa. On top of that there is a runtime performance penalty. This leads to the next layers: WebTargets and handlers. Currently DTO's are exposed (passed) to the handlers. This might be the reason for current approach? But actually situation is quite similar here. So if these would be separated also, the DTO problem would be solved. In other words, I think it would be worthwhile for the long term to make a more distinct separation in the implementation of the two API's. I could contribute, and of course also provide extended explanations and thoughts. @andrewfg, sorry for taking such a long time. Really wanted to provide some more concrete proposal before commenting on your approach. |
|
Draft notes comparing shade. shade Gen 2:
Properties:
shade Gen 3:
Properties:
Difference - for example batterykind: Gen 2:
Gen 3:
|
Hmm. I am not sure that I completely agree with you on this. The exercise with HDPowerview Gen1/2 vs Gen 3 is a very similar exercise to the one needed for Philips Hue API V1 vs CLIP 2 #13570. Originally for the Hue case I started with the intention to 'overlay' the new and old DTO's on top of each other. However in the Hue case, the DTO's, and indeed the architecture is really totally different, and for that reason I had to abandon the 'overlaying' approach and start with essentially a whole new architecture. By contrast in the HDPowerview I WAS able to maintain the 'overlaying' approach, and therefore for that reason, I am in two minds about whether I agree with you or not. Notwithstanding the above, during the writing of #13570 (which was a MASSIVE exercise) I did learn some things, that I might reverse apply to the HDPowerview case.. |
|
@jlaur : I was not aware you were studying this PR. You should have tell us. |
|
@jlaur just a couple of pointers..
|
This part could even be extracted to a separate PR which I would approve and merge immediately. I completely agree with this cleanup operation. |
|
^ |
|
^ |
|
^ |
|
^ |
IMHO you should not change any code just in order to reduce diffs. Code is as it is. :-) For that purpose commits can be used, i.e. moving files in one commit and changing them in a separate commit. Then a PR can be easier to read commit by commit. But that's more a general comment/tip. That said, the structure needs some minor tweaking to avoid checkstyle issues, for example "_v1" does not conform to naming convention because of the underscore. I don't have problems reading the changes, the reason why I cannot quickly approve this PR is because we need to discuss the architectural approach as mentioned here. As an example. let's say I want to understand what's going on from receiving a payload and until it lands in the handler. As an example, In method Shades shades = webTargets.getShades();Step into this method. It's abtract. Now find relevant implementation: Shades shades = gson.fromJson(json, Shades.class);Seems straight-forward. Hmm, but something is fishy - in protected void onReceiveUpdate(ShadeData shadeData) {
isGeneration1 = shadeData.version() == 1;Let's check the version method: public abstract int version();Okay, so now let's find the relevant implementation: public HDPowerViewWebTargetsV1(HttpClient httpClient, String ipAddress) {
super(httpClient, ipAddress, SceneV1.class, ShadeDataV1.class, ShadePositionV1.class, ScheduledEventV1.class);Let's track this in super constructor: public HDPowerViewWebTargets(HttpClient httpClient, String ipAddress, Class<? extends Scene> sceneClass,
Class<? extends ShadeData> shadeDataClass, Class<? extends ShadePosition> shadePositionClass,
Class<? extends ScheduledEvent> scheduledEventClass) {
this.httpClient = httpClient;
this.sceneClass = sceneClass;
this.shadeDataClass = shadeDataClass;
this.shadePositionClass = shadePositionClass;
this.scheduledEventClass = scheduledEventClass;
this.gson = new GsonBuilder()
// @formatter:off
.registerTypeAdapter(Scene.class, new SceneDeserializer())
.registerTypeAdapter(ShadeData.class, new ShadeDataDeserializer())
.registerTypeAdapter(ShadePosition.class, new ShadePositionDeserializer())
.registerTypeAdapter(ShadePosition.class, new ShadePositionSerializer())
.registerTypeAdapter(ScheduledEvent.class, new ScheduledEventDeserializer())
// @formatter:on
.create();
}So we have a type adapter registered, and we store a derivative of private class ShadeDataDeserializer implements JsonDeserializer<ShadeData> {
@Override
public @Nullable ShadeData deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context)
throws JsonParseException {
JsonObject jsonObject = json.getAsJsonObject();
return context.deserialize(jsonObject, shadeDataClass);
}
}For me that seems like an overly complex solution for trying to avoid a bit of duplication from two slightly overlapping, but different, API's. It's hard to read/understand (more classes to look into), hard to debug and more error-prone. We can leave this for a moment. I was planning to create a concrete example of an alternative more decoupled architectural approach, and I would still like to do that, I just need a bit of time. My hope is that it would make the discussion easier/more understandable, as code says more than a thousand words. :-) |
|
^ ... you want this instead ??? .. .. if that is the case, (which would be the way I am doing it for the Hue CLIP 2), then we would have completely different DTO's for Gen 3, and therefore also a completely new thing handler for Shades. Or? EDIT: and if the above would be the case, (i.e. completely new thing handler), then it is also a completely new bridge handler, so .. err .. basically .. a completely new binding, with no commonalities in code between the two versions. Or ?? |
Yes, exactly.
Not necessarily, but after looking at what we already have for Gen 2, I'm tended to prefer that approach. However, it would also be possible to introduce new classes (as opposed to the DTO classes) for sharing common objects, but I think we wouldn't benefit much from it. I went through all shade handler methods recently, and didn't find much of common use, i.e. most methods are quite implementation specific and tied to Gen 2 IMHO. The same goes for WebTargets which is just a controller connecting the handler with the endpoints through DTO's - and these endpoints and DTO's are different. It also contains some handling of "maintenance period", but that's probably also Gen 2-specific.
Yes and no. :-) It would still be the same binding, but the discovery would detect either Gen 2 Hub or Gen 3 Gateway and create a corresponding bridge. For Gen 3 this bridge would also contain some SSE options probably, and later there might be other differences from the current hub bridge. This doesn't mean the bridge or shade handlers cannot share code. Of course they can, and we should of course avoid any kind of duplication. But please have a look and see if you can find actual code snippet candidates for being shared? |
|
Ok. Let me think about it. But my first reaction is that probably we should delete this PR ("preparation for..") because essentially the old code should be kept as close to what it is already, and the second PR would then become "as replacement of.. resp. in addition to.." the existing code. Or ?? |
|
This PR is no longer required, so closing now. |
Background
See #12678
This is the first of (probably) three Pull Requests for integrating Hunter Douglas Generation 3 hubs into the binding (which currently only supports Generation 1 & 2 hubs).
Content of this Pull Request
The current binding has several "hub facing" DTO classes that are very specific to Generation 1 & 2 hubs. This Pull Request restructures those classes into two parts, namely..
This means that the "OH facing" parts of the code can reference the abstract base classes independently of the actual implementation of the "hub facing" classes.
Since this Pull Request provides only the Generation 1 & 2 implementation classes, the functionality should not change as a result of this PR (i.e. only the code structure has changed). And the next forthcoming Pull Request will provide additional classes that implement the Generation 3 hub specific code.
Specifics of Changes
The following classes have been restructured into base plus implementation classes..
The HDPowerViewWebTargets base class has been refactored so that it can generically support deserialization of any version of the implementation classes. i.e. the current V1 implementation classes, the forthcoming V3 implementation classes, and indeed any future version yet to come.
And various minor changes have been made in the following classes as follows..
Java Package Structure
In cases where the classes have been split into base and implementation classes, the base class remains in the package where the original combined class was located, and the split V1 implementation class is now in a ._v1 sub package. And in future the V3 implementations will be added to a ._v3 sub package.
Signed-off-by: Andrew Fiddian-Green [email protected]