-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[mqtt] Have a working Group Id #5156
Conversation
It requires a rebase now that that one is merged. |
Signed-off-by: Jochen Klein <git@jochen.susca.de>
1c53b86
to
9705a8e
Compare
final public String component; | ||
final public String nodeID; | ||
final public String objectID; | ||
final private String baseTopic; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a final property cannot be mutated from outside and this is even an immutable class.
There is no reason to hide the properties behind getters except if you don't like performance or readability or maintainability or want to make it extra hard for reviewers ^^.
Please make those fields public again and access them directly. The HaID class is meant to be immutable. That might be added to the javadoc for the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall a lot of improvements. Thanks.
A few annotations included.
Prefer immutable classes if possible. They are thread safe, easy to read and review. Don't use getters. The "object oriented programming" paradigm from the 90ties with getters/setters everywhere -> That gets my hackles up.
String groupId = channel.getGroupId(); | ||
if (groupId == null) { | ||
throw new IllegalArgumentException("Channel needs a group ID!"); | ||
public static HaID fromConfig(String baseTopic, Configuration config) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The factory pattern is like public constructors -> a fellow developer would really enjoy javadoc here.
There are also new public methods in this class, can you please add javadoc there as well? thanks
public String getTopic(String suffix) { | ||
StringBuilder str = new StringBuilder(); | ||
|
||
str.append(baseTopic).append('/').append(component).append('/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please precompute that in the constructor and store it in a private field and then just do a string concatenation: _stored_var+suffix
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this one.
This is a trde off between execution time and space.
While the mehtod is called max. a couple of times during initialization, the member will take up space for the whole runtime....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup a tradeoff. It's up to you. Maybe other fields are not used and can be dumped?
return new ChannelTypeUID(MqttBindingConstants.BINDING_ID, | ||
objectID + "_" + component + nodeID + "_" + channelID); | ||
@Override | ||
public int hashCode() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use basic types only like in this class, the java compiler will generate standard hashCode
and equals
methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing something here?
`
public class Pojo {
private final String name;
public Pojo(String name) {
this.name = name;
}
public static void main(String[] argv) {
Pojo a = new Pojo("test");
Pojo b = new Pojo("test");
System.out.println("a.hashCode() = " + a.hashCode());
System.out.println("b.hashCode() = " + b.hashCode());
System.out.println("a.equals(b) = " + a.equals(b));
}
}
does not work. And the default equals method inherited from Object is:
public boolean equals(Object obj) {
return (this == obj);
}
`
which will only use the address of the object, not any members into account.
And I added the methods, because using equals()
in the unit tests did not work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. Alright. Different in C++.
return objectid; | ||
} | ||
|
||
public void toProperties(Map<String, Object> properties) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an anti-pattern (using an argument as output).
Please call this method "appendToProperties" or alike and return the map again. And add javadoc please.
@@ -27,9 +29,31 @@ | |||
/** | |||
* The MQTT prefix topic | |||
*/ | |||
public String basetopic = "homeassistant"; | |||
private String basetopic = "homeassistant"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please make this an immutable class. final public properties only. No getters.
Your empty constructor will then assign "homeassistant".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, my tendency towards getter/setters i showing my age ;-)
Signed-off-by: Jochen Klein <git@jochen.susca.de>
@@ -161,7 +161,7 @@ public void receivedMessage(ThingUID connectionBridge, MqttBrokerConnection conn | |||
|
|||
Map<String, Object> properties = new HashMap<>(); | |||
HandlerConfiguration handlerConfig = topicParts.toHandlerConfiguration(); | |||
handlerConfig.toProperties(properties); | |||
handlerConfig.appendToProperties(properties); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it like so: properties = handlerConfig.appendToProperties(properties);
I'm not sure if the java compiler is smart enough to figure out that it means the same (in c++ that's possible). But for someone reading the code it is more obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, should have thought about this...
Its too late after a working day...
Signed-off-by: Jochen Klein <git@jochen.susca.de>
Signed-off-by: Jochen Klein <git@jochen.susca.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor comments, would be nice if you could fix them now or in a follow up PR
public final String nodeID; | ||
public final String objectID; | ||
|
||
private final String _topic; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a correct name according to the Java / openHAB coding guidelines
channelState = new ChannelState( | ||
ChannelConfigBuilder.create().withRetain(retain).withStateTopic(state_topic) | ||
.withCommandTopic(command_topic).build(), | ||
channelUID, valueState, channelStateUpdateListener); | ||
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wrong but of course not stopping this PR from being merged
Signed-off-by: Jochen Klein <git@jochen.susca.de>
move info from groupd id to group config Signed-off-by: Jochen Klein <git@jochen.susca.de> Signed-off-by: Pshatsillo <pshatsillo@gmail.com>
move info from groupd id to group config Signed-off-by: Jochen Klein <git@jochen.susca.de> Signed-off-by: Maximilian Hess <mail@ne0h.de>
Use the device.unique_id as a channel group id, if availble. Use nodeId, ObjectId and component as fallback.
Write the information about nodeId and objectId into the channel config to avoid the need to recover the information from the group id.
Also make the channel configuration read only in the GUI. I changes are needed, then a GenericMQTTThing should be used and not a HomeAssistantThing.
With this PR I am having functional autodisovery for tasmota for the already supported compoenents / features.
@davidgraeff Sorry, but I forgot this PR in my other list, because it was basically already prepared.
This PR is based on #4990