Skip to content
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

[enocean] D2-50 EEP remove extra channels and fix warnings #17531

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lovery
Copy link
Contributor

@lovery lovery commented Oct 9, 2024

Description

Improvement: Fix 7 warnings on building a jar:

  • Null comparison always yields false... - EEPFactory.java:[54,17], EEPFactory.java:[67,17], EnOceanBaseSensorHandler.java:[158,13]
  • Dead code (caused by the above warinings) - EEPFactory.java:[54,29], EEPFactory.java:[67,29], EnOceanBaseSensorHandler.java:[158,41]
  • Potential null pointer access - EnOceanTransceiver.java:[451,13]

Improvement: remove channels which are not supported by device using EEP D2-50-00, D2-50-01, D2-50-10, D2-50-11.
Before this PR choosing either of the above EEP doesn't change anything in the channels list, it looked like all the devices support all channels, but according to the specification they don't. For reference check EnOcean specification on page 236

Screenshot from 2024-10-09 10-51-00

Test jar file

Signed-off-by: Zhivka Dimova <zhivka.dimova@myforest.net>
Warning list:
* Null comparison always yields false
* Potential null pointer access: The method computeIfAbsent(Long, Function) may return null

Signed-off-by: Zhivka Dimova <zhivka.dimova@myforest.net>
@lovery lovery requested a review from fruggy83 as a code owner October 9, 2024 10:03
@lsiepel lsiepel added the enhancement An enhancement or new feature for an existing add-on label Oct 9, 2024
@jlaur jlaur changed the title [enocean]: D2-50 EEP remove extra channels and fix warnings [enocean] D2-50 EEP remove extra channels and fix warnings Oct 9, 2024
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, otherwise LGTM

Comment on lines -54 to +55
if (cl == null) {
if (Objects.isNull(cl)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the benefit of this change? To me the check was perfectly fine as is and even better readable. (same for other occurences)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to lower the amount of warnings on building and with the code before I was getting

[WARNING] /home/zhivka/workspace/openhab/openhab-addons/bundles/org.openhab.binding.enocean/src/main/java/org/openhab/binding/enocean/internal/eep/EEPFactory.java:[54,17] Null comparison always yields false: The variable cl cannot be null at this location
[WARNING] /home/zhivka/workspace/openhab/openhab-addons/bundles/org.openhab.binding.enocean/src/main/java/org/openhab/binding/enocean/internal/eep/EEPFactory.java:[54,29] Dead code

After my change those warnings didn't appear. Do you know better way to fix those warnings?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the getEEPClass() annotation it will never return null, so the compiler knows this is dead code.
So either these checks can be removed OR the annotation should be changed. I'm 99% sure its the first.

@@ -155,7 +156,7 @@ public void packetReceived(BasePacket packet) {
ERP1Message msg = (ERP1Message) packet;

EEPType localReceivingType = receivingEEPTypes.get(msg.getRORG());
if (localReceivingType == null) {
if (Objects.isNull(localReceivingType)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See other comment about this null check. I would prefer to revert this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants