-
Notifications
You must be signed in to change notification settings - Fork 764
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
feat: handle sleep behavior of MCU2 upgraded cars #4453
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for teslamate ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
I am having trouble making sense of this PR. It started off simple (with the original first two commits from the first PR) and has become rather complicated, and IMHO hard to understand. And I am wondering if this complexity is really required. Then there are some changes when in this PR that don't belong here; for example the changes to delete the "models2" entries. I think this might have been included in error. There are some changes though that would be easy to apply separately, for example improvements to the log messages. |
Agree, I do have trouble reviewing. The comments helped a bit. |
850eb3f
to
dee7726
Compare
dee7726
to
d2999d2
Compare
d2999d2
to
f951d44
Compare
Not sure where the model2 changes are coming from. They are not in the original https://github.com/teslamate-org/teslamate/pull/3262/files Maybe it wasn't synced when this PR was made? The commit history of the old #3262 is long and a bit messed up. Might be easier to only look at the end result. Which again as you mention is a bit complex :( The shortest way to sum it up, might actually just be the description I made on fake_online_state variable:
The complexity comes in because online all of a sudden might not be online. This means that the entire online state handling and stream is re-done, and the tests (or as I see it, the API mock used for tests) does not support the fact that a Stream is required. The ultimate solution would be that the car or Tesla servers would hide the fake online. |
changes from @micves from #3262, fixes #3084