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

[MercedesMe] Switch to Mercedes App SDK #15628

Merged
merged 128 commits into from
Jun 4, 2024
Merged

Conversation

weymann
Copy link
Contributor

@weymann weymann commented Sep 19, 2023

The current implementation of MercedesMe binding is based on Developer API from Mercedes. Unfortunately they quit supporting this on short hand notice end of August. Confirmed by me and recognized by the Community - binding is not working anymore!

New solution is based on Mercedes App SDK. Same as Homeassitant Mercedes integration which is still working.
This is a breaking change but it's needed to have this binding running again. I see no way Bridges and Vehicles can be reused due to different configurations.

@wborn
Copy link
Member

wborn commented Sep 19, 2023

I need advice how to exclude tool generated Google protobuf classes from checkstyle

IIRC we only check our own classes in the org.openhab package. So you can move generated or third party code to another package or add it to the suppressions.

@weymann
Copy link
Contributor Author

weymann commented Sep 19, 2023

@wborn
Moving protobuf classes out of org.openhab package doesn't work. Still get all errors

[ERROR] com.daimler.mbcarkit.proto.VehicleCommands.java:[6]
An author tag is missing

Maybe I should be more specific: It's generated sources, not directly class files.

@weymann weymann requested a review from a team as a code owner September 26, 2023 19:23
@wborn wborn added the work in progress A PR that is not yet ready to be merged label Oct 2, 2023
@weymann weymann changed the title [WIP][MercedesMe] Switch to Mercedes App SDK [MercedesMe] Switch to Mercedes App SDK Nov 24, 2023
@weymann
Copy link
Contributor Author

weymann commented Nov 25, 2023

Work in progress removed.
Binding looks pretty stable after testing by community

@openhab/maintainers still struggeling with proto java classes provided by Mercedes / Daimler which are causing many checkstyle issues.
I moved proto files to com.daimler.* package but checkstyle is still preventing a clean run

Resolved in latest commits

@wborn
Copy link
Member

wborn commented Nov 25, 2023

I had a look and currenty checkstyle is configured to check everything in src/main/java.

Some other add-ons have third party sources in src/3rdparty/java like flicbutton.
The build-helper-maven plugin is used in pom.xml to add those sources:

If that doesn't help, the last resort is to add a suppression for those packages in suppressions.xml.

@lsiepel
Copy link
Contributor

lsiepel commented Feb 19, 2024

@weymann this is marked as WIP for some time and that is perfectly fine. With 150+ changed files, it would take multiple review rounds i guss and some extensive testing to prevent regressions. Is there anything you need to proceed?

@weymann
Copy link
Contributor Author

weymann commented Feb 27, 2024

@weymann this is marked as WIP for some time and that is perfectly fine. With 150+ changed files, it would take multiple review rounds i guss and some extensive testing to prevent regressions. Is there anything you need to proceed?

Please check the initial comment of this PR.
Regression already took place at August 23 when Mercedes simply quit the support of the API.

Testing of this implementaion started Sep 23 here and there are ~80 comments afterwards which came into bugfixing.

WIP removed in comment from Nov 23 here

@lsiepel
Copy link
Contributor

lsiepel commented Feb 27, 2024

@weymann this is marked as WIP for some time and that is perfectly fine. With 150+ changed files, it would take multiple review rounds i guss and some extensive testing to prevent regressions. Is there anything you need to proceed?

Please check the initial comment of this PR. Regression already took place at August 23 when Mercedes simply quit the support of the API.
Testing of this implementaion started Sep 23 here and there are ~80 comments afterwards which came into bugfixing.

WIP removed in comment from Nov 23 here

It still has the WIP label. I’ll remove that. Once the conflicts are also solved a review can take place

@lsiepel lsiepel added (potentially) not backward compatible enhancement An enhancement or new feature for an existing add-on and removed work in progress A PR that is not yet ready to be merged labels Feb 27, 2024
@lolodomo
Copy link
Contributor

163 000 lines of code !
Who will have the courage to start the review of such a big PR ?

@weymann
Copy link
Contributor Author

weymann commented Apr 23, 2024

163 000 lines of code ! Who will have the courage to start the review of such a big PR ?

@lolodomo Keep calm and start review!

code without comments and blank lines ~111K

image

weymann added 10 commits May 3, 2024 13:26
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
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.

Only the /build/gen/com/daimler/mbcarkit files are left to be moved.

Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
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.

Last one ^^

Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
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 for the work on adapting this binding to the new Mercedes SDK.
LGTM.
Do you need some additional days to test before merge or is it well tested after all changes we made?

@weymann
Copy link
Contributor Author

weymann commented May 21, 2024

Do you need some additional days to test before merge or is it well tested after all changes we made?

You can leave it open for one or two weeks as you want, It's running on my machine and published on Marketplace. If I don't provide any more commits in this time it should be fine.
Most problematic issue reported during this review is solved and confirmed by the user.

@lsiepel
Copy link
Contributor

lsiepel commented May 21, 2024

Do you need some additional days to test before merge or is it well tested after all changes we made?

You can leave it open for one or two weeks as you want, It's running on my machine and published on Marketplace. If I don't provide any more commits in this time it should be fine. Most problematic issue reported during this review is solved and confirmed by the user.

Oke, will do. Could you create a upgrade warning in openHAB/distro ?

https://github.com/openhab/openhab-distro/blob/328f2a34fc0c1f5ae492b9ecad89fc613fa63f69/distributions/openhab/src/main/resources/bin/update.lst

@weymann
Copy link
Contributor Author

weymann commented May 21, 2024

Oke, will do. Could you create a upgrade warning in openHAB/distro ?

Please assist.
As stated in PR description public Mercedes Me API isn't available anymore. So there's no way to avoid these changes. What shall I document besides readme chnages?

@lsiepel
Copy link
Contributor

lsiepel commented May 22, 2024

Oke, will do. Could you create a upgrade warning in openHAB/distro ?

Please assist. As stated in PR description public Mercedes Me API isn't available anymore. So there's no way to avoid these changes. What shall I document besides readme chnages?

It is an extra (more active) notice so that users are aware they have to adapt their configuration. Recreate Thing and or update item channel links. This is not a drop in replacement

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.

This one slipped through.

@@ -1,218 +0,0 @@
# add-on
Copy link
Contributor

Choose a reason for hiding this comment

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

From my understanding there should not be any changes to this file from this PR. You can manually save/backup the file and manually upload it after this PR is merged into crowdin.

Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
@weymann
Copy link
Contributor Author

weymann commented Jun 2, 2024

This one slipped through.

Yes, I messed up previous revert - some special charachters were destroyed while copy/paste. Took now original file from main branch and pushed it. Now file isn't listed any longer in file changes

@weymann weymann requested a review from lsiepel June 3, 2024 19:58
@lsiepel lsiepel merged commit 09a22e5 into openhab:main Jun 4, 2024
5 checks passed
@lsiepel lsiepel added this to the 4.2 milestone Jun 4, 2024
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Jun 16, 2024
* add protocol buffer definitions
* oauth rework
* websocket introduction

Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
psmedley added a commit to psmedley/openhab-addons that referenced this pull request Jun 16, 2024
@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/mercedes-me-binding/136852/206

@weymann weymann deleted the mercedes-sdk branch September 8, 2024 15:23
pgfeller pushed a commit to pgfeller/openhab-addons that referenced this pull request Sep 29, 2024
* add protocol buffer definitions
* oauth rework
* websocket introduction

Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
* add protocol buffer definitions
* oauth rework
* websocket introduction

Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
* add protocol buffer definitions
* oauth rework
* websocket introduction

Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
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 (potentially) not backward compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants