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

[mybmw] Upgrade to new BMW API #14452

Merged
merged 79 commits into from
Dec 14, 2023
Merged

[mybmw] Upgrade to new BMW API #14452

merged 79 commits into from
Dec 14, 2023

Conversation

martingrassl
Copy link
Contributor

@martingrassl martingrassl commented Feb 19, 2023 •
edited
Loading

This is mainly a bugfix PR to resolve the failing backend requests.

To make it work the code has been refactored and due to API changes some improvements could be made. These include:

  • (improvement) fingerprint generation: You can take a look at the README how to create a fingerprint more conveniently
  • (change) changed channel: charge-info has been renamed to charge-remaining
  • (improvement) added channels: estimated-fuel-l-100km and estimated-fuel-mpg which calculates the estimated fuel consumption based on the range and remaining fuel liters - unfortunately such a calculation is not available for EVs as there is no information about the capacity of the battery.
  • (improvement) added channel last-fetched: the last-updated timestamp is showing by when the last update of the vehicle happened. As right now you can not see from the channels if a thing is offline due to connection issues, you can check now if last-fetched is more than 5 minutes ago to identify an issue
  • (fixed) remote command typos fixed
  • (improvement) Finnish translation added

Co-authored-by: Mark Herwege [email protected]

Fixes #14065

Also-by: Mark Herwege [email protected]
Signed-off-by: Martin Grassl [email protected]

Martin and others added 4 commits February 19, 2023 21:58
@martingrassl
[mybmw] fix not working binding due to API update
f026cd2 
to make it work the code has been refactored and due to API changes some
improvements could be made. These include:
- (improvement) fingerprint generation: You can
  take a look at the README how to create a
  fingerprint more conveniently.
- (change) changed channel: charge-info has been
  renamed to charge-remaining
- (improvement) added channels:
  estimated-fuel-l-100km and estimated-fuel-mpg
  which calculates the estimated fuel consumption
  based on the range and remaining fuel liters
  - unfortunately such a calculation is not available
  for EVs as there is no information about the capacity of the battery.
- (improvement) added channel last-fetched:
  the last-updated timestamp is showing by when
  the last update of the vehicle happened. As right
  now you can not see from the channels if a thing
  is offline due to connection issues, you can check
  now if last-fetched is more than 5 minutes ago to identify an issue
- (fixed) remote command typos fixed

Co-authored-by: Mark Herwege <[email protected]>

Fixes openhab#14065

Also-by: Mark Herwege <[email protected]>
Signed-off-by: Martin Grassl <[email protected]>
@martingrassl
[mybmw] fix case sensitive brand in request header
544272b 
Co-authored-by: Mark Herwege [email protected]

Also-by: Mark Herwege [email protected]
Signed-off-by: Martin Grassl [email protected]
Signed-off-by: Martin Grassl <[email protected]>
@martingrassl
[mybmw] added Finnish translations
7125555 
Co-authored-by: Jari Likonen <[email protected]>

Also-by: Jari Likonen <[email protected]>
Signed-off-by: Martin Grassl <[email protected]>
@martingrassl
[mybmw] update Copyright header
f94b448 
Signed-off-by: Martin Grassl <[email protected]>
@jlaur jlaur changed the title 14065 upgrade bmw api 40 [mybmw] Upgrade bmw api 40 Feb 19, 2023
@jlaur jlaur added the enhancement An enhancement or new feature for an existing add-on label Feb 19, 2023
@jlaur
Copy link
Contributor

jlaur commented Feb 19, 2023

@martingrassl - thank you! I have briefly looked at the README, and I noticed that you have reverted the fixes made in #13866. So please go through that and revert those changes.

@martingrassl
Copy link
Contributor Author

@martingrassl - thank you! I have briefly looked at the README, and I noticed that you have reverted the fixes made in #13866. So please go through that and revert those changes.

@jlaur Oh thanks for pointing that out. I didn't see that in my comparisons. I'll change it tomorrow, sorry for that.

@martingrassl martingrassl changed the title [mybmw] Upgrade bmw api 40 [mybmw] Upgrade bmw api Feb 19, 2023
@martingrassl
Copy link
Contributor Author

@martingrassl - thank you! I have briefly looked at the README, and I noticed that you have reverted the fixes made in #13866. So please go through that and revert those changes.

Hi @jlaur, I fixed the markdown issues according to the changes of #13866. I installed the markdownlint extension to VSCode and it looks like it applies the same rules. I'm just wondering if this linting check could be somehow integrated into the Maven process so that it fails when there is an issue like with spotless for Java code...

@jlaur
Copy link
Contributor

jlaur commented Feb 20, 2023

I'm just wondering if this linting check could be somehow integrated into the Maven process so that it fails when there is an issue like with spotless for Java code...

See here: #13858

@weymann
Copy link
Contributor

weymann commented Mar 19, 2023

Thx for taking the task to bring this binding in working mode again. As stated in the communnity I don't have a BMW anymore so I'm not able test the functionality anymore-
@jlaur Will you add @martingrassl as codeowner too?

@jlaur
Copy link
Contributor

jlaur commented Mar 19, 2023

Thx for taking the task to bring this binding in working mode again. As stated in the communnity I don't have a BMW anymore so I'm not able test the functionality anymore- @jlaur Will you add @martingrassl as codeowner too?

@weymann - since you just approved of this, @martingrassl - if you agree, please add yourself to the CODEOWNERS file in context of this PR. 🙂

weymann
weymann requested changes Mar 19, 2023
View reviewed changes
Copy link
Contributor

@weymann weymann left a comment

Choose a reason for hiding this comment

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

Modify CODEOWNERS file and put yourself as codeowner. You can also remove me due to the fact I'm not in charge anymore.

@martingrassl
[mybmw] add and replace code owners
0227b6a 
Also-by: Mark Herwege <[email protected]>
Signed-off-by: Martin Grassl <[email protected]>
@martingrassl martingrassl requested a review from a team as a code owner March 20, 2023 20:49
@martingrassl
Copy link
Contributor Author

@jlaur I added @mherwege as well as he did a great job in supporting me bringing the add-on back to life. And he has good ideas for further enhancements :)

@martingrassl
[mybmw] add and replace code owners - fix name
eb3d5f8 
sorry for the confusion - GitHub showed an error unknown user...

Also-by: Mark Herwege <[email protected]>
Signed-off-by: Martin Grassl <[email protected]>
@jlaur
Copy link
Contributor

jlaur commented Mar 20, 2023

I added @mherwege as well as he did a great job in supporting me bringing the add-on back to life. And he has good ideas for further enhancements :)

If that's okay with @mherwege, great. 👍

martingrassl and others added 5 commits December 13, 2023 12:47
@martingrassl @jlaur
Update bundles/org.openhab.binding.mybmw/src/main/java/org/openhab/bi…
c3389d4 
…nding/mybmw/internal/handler/VehicleHandler.java

Co-authored-by: Jacob Laursen <[email protected]>
Signed-off-by: Martin Grassl <[email protected]>
@martingrassl @jlaur
Update bundles/org.openhab.binding.mybmw/src/main/java/org/openhab/bi…
b8757ad 
…nding/mybmw/internal/handler/VehicleHandler.java

Co-authored-by: Jacob Laursen <[email protected]>
Signed-off-by: Martin Grassl <[email protected]>
@martingrassl @jlaur
Update bundles/org.openhab.binding.mybmw/README.md
53c1dd1 
Co-authored-by: Jacob Laursen <[email protected]>
Signed-off-by: Martin Grassl <[email protected]>
@martingrassl @jlaur
Update bundles/org.openhab.binding.mybmw/README.md
5ed0773 
Co-authored-by: Jacob Laursen <[email protected]>
Signed-off-by: Martin Grassl <[email protected]>
@martingrassl @jlaur
Update bundles/org.openhab.binding.mybmw/src/main/java/org/openhab/bi…
06ca7af 
…nding/mybmw/internal/handler/auth/MyBMWTokenController.java

Co-authored-by: Jacob Laursen <[email protected]>
Signed-off-by: Martin Grassl <[email protected]>
@martingrassl
Copy link
Contributor Author

just in case you missed it, there are still a few minor comments open from this review iteration: #14452 (review)

Hmmm but I fixed all of them? Can you tell me what you exactly mean?

I checked them all and left those open that are not addressed by either a commit or comment. So I think you can just go through them. Remember to "show hidden". 🙂

Ok I missed the hidden ones. I was not aware that GH hides unresolved ones as well... thanks for pointing this out. I'll fix them today evening.

@martingrassl
[mybmw] fix compile error
bc8ae15 
Signed-off-by: Martin Grassl <[email protected]>
@martingrassl
[mybmw] extract strings to constants
1c9325b 
Signed-off-by: Martin Grassl <[email protected]>
@martingrassl
[mybmw] cleanup comments
50857a2 
Signed-off-by: Martin Grassl <[email protected]>
@martingrassl
[mybmw] add description of return value
dbf9d8d 
Signed-off-by: Martin Grassl <[email protected]>
@martingrassl
[mybmw] fix Javadoc
c8bf8bf 
Signed-off-by: Martin Grassl <[email protected]>
@martingrassl
[mybmw] catch InterruptedException properly
9814602 
Signed-off-by: Martin Grassl <[email protected]>
@martingrassl
[mybmw] revert change of some props
7019a00 
Signed-off-by: Martin Grassl <[email protected]>
@martingrassl
[mybmw] revert deletion of de translations
7054713 
Signed-off-by: Martin Grassl <[email protected]>
@martingrassl
[mybmw] fix missing line break at end of file
1b971b1 
Signed-off-by: Martin Grassl <[email protected]>
jlaur
jlaur approved these changes Dec 14, 2023
View reviewed changes
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

LGTM. @martingrassl - thanks again for the contribution! Is the PR ready for being merged from your perspective?

@martingrassl
Copy link
Contributor Author

LGTM. @martingrassl - thanks again for the contribution! Is the PR ready for being merged from your perspective?

@jlaur from my perspective it is ready for a long time - it would be a great Christmas present if it would be merged into 4.1.0 ;) thanks for your thorough review and the time you put into doing that, your comments helped me understanding the matter a bit better!

Now it just looks like @lsiepel has to approve the review, right?

@jlaur
Copy link
Contributor

jlaur commented Dec 14, 2023

Now it just looks like @lsiepel has to approve the review, right?

It seems all comments from @lsiepel are resolved, and that there are no further comments: #14452 (review) (and btw, thanks for the review, @lsiepel 👍)

So we can merge, and get this binding working again.

@jlaur jlaur merged commit 4f84c48 into openhab:main Dec 14, 2023
3 checks passed
@jlaur jlaur added this to the 4.1 milestone Dec 14, 2023
kaikreuzer pushed a commit to openhab/openhab-distro that referenced this pull request Dec 21, 2023
@martingrassl
[mybmw] added change info for mybmw binding (#1581)
9273d98 
this PR depends on openhab/openhab-addons#14452

Signed-off-by: Martin Grassl <[email protected]>
@martingrassl martingrassl deleted the 14065_upgrade_BMW_API_40 branch December 22, 2023 05:41
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
@martingrassl @austvik
[mybmw] Upgrade to new BMW API (openhab#14452)
245825d 
* [mybmw] fix not working binding due to API update

to make it work the code has been refactored and due to API changes some
improvements could be made. These include:
- (improvement) fingerprint generation: You can
  take a look at the README how to create a
  fingerprint more conveniently.
- (change) changed channel: charge-info has been
  renamed to charge-remaining
- (improvement) added channels:
  estimated-fuel-l-100km and estimated-fuel-mpg
  which calculates the estimated fuel consumption
  based on the range and remaining fuel liters
  - unfortunately such a calculation is not available
  for EVs as there is no information about the capacity of the battery.
- (improvement) added channel last-fetched:
  the last-updated timestamp is showing by when
  the last update of the vehicle happened. As right
  now you can not see from the channels if a thing
  is offline due to connection issues, you can check
  now if last-fetched is more than 5 minutes ago to identify an issue
- (fixed) remote command typos fixed

Fixes openhab#14065

Also-by: Mark Herwege <[email protected]>
Signed-off-by: Martin Grassl <[email protected]>
Signed-off-by: Jørgen Austvik <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maniac103 maniac103 maniac103 left review comments

@lsiepel lsiepel lsiepel requested changes

@jlaur jlaur jlaur approved these changes

@weymann weymann weymann approved these changes

@ntruchsess ntruchsess Awaiting requested review from ntruchsess ntruchsess 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.1
Development

Successfully merging this pull request may close these issues.

[MyBMW] Bridge is not able to connect
8 participants
@martingrassl @jlaur @weymann @openhab-bot @lsiepel @mherwege @TheTrueRandom @maniac103