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.

Sign up for GitHub

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

Jump to bottom

[tesla] Add null annotations #17582

Merged
merged 5 commits into from
Dec 8, 2024
Merged

[tesla] Add null annotations #17582

merged 5 commits into from
Dec 8, 2024

Conversation

lsiepel
Copy link
Contributor

@lsiepel lsiepel commented Oct 17, 2024

  • Fix null annotations
  • Fix compile warnings
  • Updated and moved DTO's to dto folder

Only form code perspective, needs some testing to be sure no regression occures.

Test jar 4.3.0 (should also work with 4.2.x) https://1drv.ms/u/s!AnMcxmvEeupwj4oOcmeYDamnLAcyAg?e=k2nzcw

@lsiepel
First pass
c2c4595 
Signed-off-by: Leo Siepel <[email protected]>
@lsiepel
Pass 2
1117ef6 
Signed-off-by: Leo Siepel <[email protected]>
@lsiepel
Final pass
e2e9154 
Signed-off-by: Leo Siepel <[email protected]>
@lsiepel lsiepel added enhancement An enhancement or new feature for an existing add-on additional testing preferred The change works for the pull request author. A test from someone else is preferred though. labels Oct 17, 2024
@lsiepel lsiepel mentioned this pull request Oct 17, 2024
Open
@lsiepel
Copy link
Contributor Author

lsiepel commented Oct 25, 2024

@kaikreuzer would you be able to test this binding? As you seem one of the few that not be forced to use the fleet api yet.

@kaikreuzer
Copy link
Member

Hey @lsiepel! Thanks for the clean-ups! I've installed this version on my production system and will watch it over the day tomorrow. So far, it seems to work and I do not see any issues in my logs. Will keep you posted.

@kaikreuzer
Copy link
Member

Unfortunately, I do not seem to receive any updates anymore - so I would say that this PR seems to break the functionality.
I'd need more time to deeply look into it in order to debug the issue...

@lsiepel
Copy link
Contributor Author

lsiepel commented Oct 28, 2024

ook into it in order to debug the issue...

Too bad, looking at the code again i can't spot the obvious error. If you can provide debug logs and your observations i can try to look at it. But maybe it is simpler for you to just debug it when having the car connected.

kaikreuzer
kaikreuzer requested changes Dec 8, 2024
View reviewed changes
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Thanks @lsiepel, I finally got it running (my local development env was broken due to openhab/openhab-distro#1697, which I only figured out today.

It seems that your code works well after all - there is just one small bug that causes it to break. Once this is fixed, it looks good!

...binding.tesla/src/main/java/org/openhab/binding/tesla/internal/protocol/dto/VehicleData.java Outdated Show resolved Hide resolved
@lsiepel @kaikreuzer
Update bundles/org.openhab.binding.tesla/src/main/java/org/openhab/bi…
086630b 
…nding/tesla/internal/protocol/dto/VehicleData.java

Co-authored-by: Kai Kreuzer <[email protected]>
Signed-off-by: lsiepel <[email protected]>
@lsiepel lsiepel removed the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Dec 8, 2024
@lsiepel lsiepel marked this pull request as ready for review December 8, 2024 12:20
@lsiepel lsiepel requested a review from kgoderis as a code owner December 8, 2024 12:20
@lsiepel
Copy link
Contributor Author

lsiepel commented Dec 8, 2024

Done, I leave it to you if you want to merge it for 4.3.0 or not.

@lsiepel lsiepel requested a review from kaikreuzer December 8, 2024 12:22
kaikreuzer
kaikreuzer approved these changes Dec 8, 2024
View reviewed changes
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Let's merge, thanks!

@kaikreuzer kaikreuzer merged commit f39415b into openhab:main Dec 8, 2024
5 checks passed
@kaikreuzer kaikreuzer added this to the 4.3 milestone Dec 8, 2024
@lsiepel lsiepel deleted the tesla-sat branch December 8, 2024 16:26
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Dec 16, 2024
@lsiepel @matchews
[tesla] Add null annotations (openhab#17582)
c78df50 
Signed-off-by: Leo Siepel <[email protected]>
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/tesla-binding-exception-occurred-while-parsing-data/161012/8

@jlaur jlaur mentioned this pull request Dec 25, 2024
Merged
cipianpascu pushed a commit to cipianpascu/openhab-addons that referenced this pull request Jan 2, 2025
@lsiepel
[tesla] Add null annotations (openhab#17582)
5dfde43 
Signed-off-by: Leo Siepel <[email protected]>
Signed-off-by: Ciprian Pascu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaikreuzer kaikreuzer kaikreuzer approved these changes

@kgoderis kgoderis Awaiting requested review from kgoderis kgoderis is a code owner

Assignees
No one assigned
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
3 participants
@lsiepel @kaikreuzer @openhab-bot