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

[miio] Correct channel ambientBrightness to type Dimmer for Yeelight Screen Light Bar #13554

Merged
merged 5 commits into from
Oct 30, 2022

Conversation

michiii1337
Copy link
Contributor

@michiii1337 michiii1337 commented Oct 15, 2022

changed Datatype from "Number" to "Dimmer" at channel "ambientBrightness" same issue #9936 (but already fixed)

Signed-off-by: Michael Schmidts michael.schmidts@online.de

@michiii1337 michiii1337 requested a review from marcelrv as a code owner October 15, 2022 19:10
@lolodomo lolodomo changed the title Update yeelink.light.light15.json [miio] Update yeelink.light.light15.json Oct 16, 2022
Copy link
Contributor

@marcelrv marcelrv left a comment

Choose a reason for hiding this comment

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

Hi @michiii1337
I recognize the issue with the dimmer vs number. I recall the problem is that I wanted to make it 2 ways, meaning that setting the dimming level in the app is also reflected in the dimming level in OH. As the device reports the level in whole number I believe it did not work well which is why I made it a number.

Can you test on your end and confirm that setting the dimming level via the app is properly reflected in OH.
If not, we prob need to add some calc to multiply/divided by 100... which is nowadays supported by the binding so I expect doable.

Also would be great to check some of the other yeelights if you have a chance for the same matter.
Lastly, once we have confirmed it all working right, please run the readme maker (in the test folder) to update the readme documentation accordingly.

@michiii1337
Copy link
Contributor Author

I tested the dimming level via app and openhab. When the datatype is set to Dimmer, both in Xiaomi Home app and Openhab is the same/correct value. Unfortunately, I have no other Yeelights so I can only test this type.

@michiii1337
Copy link
Contributor Author

the readme is now up to date. Hope I did everything right as this is my first pullreqest.

@marcelrv
Copy link
Contributor

I think code wise it is fine.
There are some oddities wrt to the automatic generated readme but they are not caused by you and should be fixed by translating the relevant values and than it fixes itself

There is a commit signoff issue with 2 of the commits. You can try to 'squash' in eclipse and update the the commit comment to include the signoff-by (like you did in your last commit)
Or if whoever is merging this agrees you can just click accept DCO and it will be fine as well. (I don't have rights to merge the PR)

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.

Thanks! There seem to be some character set issues that would need to be resolved, and also the DCO check must pass.

@michiii1337 michiii1337 requested a review from jlaur October 28, 2022 16:55
@michiii1337
Copy link
Contributor Author

I fixed the characters in the readme. How can i fix the DCO? I can not edit the 3 commits without my sign.

@marcelrv
Copy link
Contributor

You can try to 'squash' in eclipse and update the the commit comment to include the signoff-by (like you did in your last commit)

@michiii1337
Copy link
Contributor Author

i tried already to squash the commits but i received a error message: "Unable to squash. Squashing replays all commits up to the last one required for the squash. A merge commit cannot exist among those commits"

@jlaur
Copy link
Contributor

jlaur commented Oct 28, 2022

I fixed the characters in the readme. How can i fix the DCO? I can not edit the 3 commits without my sign.

You could try to click "Details" in the DCO check line and follow the steps described, or you could have a look at this:
https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History

changed Datatype form "Number" to "Dimmer" at channel "ambientBrightness"
same issue openhab#9936 (but already fixed)

Signed-off-by: Michi <michael.schmidts@online.de>
@michiii1337
Copy link
Contributor Author

i followed the instructions under "Details" and now the DCO failed again... is it easier to start from zero and make an new PR?

@marcelrv
Copy link
Contributor

marcelrv commented Oct 29, 2022

Maybe what's working is to do first a rebase, than I think it will allow you to do the squash.
Making a new branch from current and copy the few changes is indeed also a way

@michiii1337
Copy link
Contributor Author

i have done a rebase and i got the same error message when i try to squash

@michiii1337
Copy link
Contributor Author

ok, now i have done a rebase. before that i seem to have done only a merge. Sorry for the many questions and problem in the PR I am still learning. Hope now the commits can be merged.

@jlaur
Copy link
Contributor

jlaur commented Oct 29, 2022

ok, now i have done a rebase

I'm sorry, but that did no go well, as there are now changes from other bindings included. Perhaps at this point it would be best to:

  • Backup your changes files.
  • Delete your branch locally.
  • Create a new branch from main with the same name as the deleted branch.
  • Apply your changes again (by copying the changed files).
  • Force push the new branch.

@michiii1337
Copy link
Contributor Author

my branch is "main". if i delete this branch how should i create new branch from main?

@jlaur
Copy link
Contributor

jlaur commented Oct 29, 2022

my branch is "main". if i delete this branch how should i create new branch from main?

Ah, sorry, I didn't see that. Assuming your fork is new and you don't have other branches to keep, for this PR I would adapt the plan to:

  • Backup your repository.
  • Delete your fork.
  • Create a new fork.
  • Apply your changes again (by copying the changed files).
  • Commit again to main while making sure signoff is correct.
  • Force push main.

I don't know if it will work, but it might be worth a shot. After that, I would then suggest to recreate the fork again, and make sure you don't commit anything to main, as it will most likely cause problems for you. Instead you should create a new branch, and use this when creating a PR.

@Hilbrand
Copy link
Member

It looks like only 1 commit is wrong. You can also just remove that one commit. Use git rebase -i HEAD~6. This will open an editor with the list of commits. Replace pick with d on the commit you want to remove. Than save the file. After that do a push force.

michiii1337 and others added 4 commits October 30, 2022 00:58
Signed-off-by: Michael Schmidts michael.schmidts@online.de
Signed-off-by: Michi <michael.schmidts@online.de>
Signed-off-by: Michael Schmidts <michael.schmidts@online.de>
Signed-off-by: Michael Schmidts michael.schmidts@online.de
Signed-off-by: Michi <michael.schmidts@online.de>
Signed-off-by: Michael Schmidts <michael.schmidts@online.de>
Signed-off-by: Michael Schmidts michael.schmidts@online.de
Signed-off-by: Michael Schmidts <michael.schmidts@online.de>
Signed-off-by: Michael Schmidts michael.schmidts@online.de
Signed-off-by: Michael Schmidts <michael.schmidts@online.de>
@michiii1337
Copy link
Contributor Author

Thanks @Hilbrand and @jlaur removed the wrong commit.

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.

Thank you!

@jlaur jlaur merged commit 6024eb8 into openhab:main Oct 30, 2022
@jlaur jlaur added this to the 3.4 milestone Oct 30, 2022
@jlaur jlaur changed the title [miio] Update yeelink.light.light15.json [miio] Correct channel ambientBrightness to type Dimmer for Yeelight Screen Light Bar Oct 30, 2022
@jlaur jlaur added the bug An unexpected problem or unintended behavior of an add-on label Oct 30, 2022
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
* Update yeelink.light.light15.json

changed Datatype form "Number" to "Dimmer" at channel "ambientBrightness"
same issue openhab#9936 (but already fixed)

Signed-off-by: Michi <michael.schmidts@online.de>
borazslo pushed a commit to borazslo/openhab-mideaac-addon that referenced this pull request Jan 8, 2023
* Update yeelink.light.light15.json

changed Datatype form "Number" to "Dimmer" at channel "ambientBrightness"
same issue openhab#9936 (but already fixed)

Signed-off-by: Michi <michael.schmidts@online.de>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
* Update yeelink.light.light15.json

changed Datatype form "Number" to "Dimmer" at channel "ambientBrightness"
same issue openhab#9936 (but already fixed)

Signed-off-by: Michi <michael.schmidts@online.de>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
* Update yeelink.light.light15.json

changed Datatype form "Number" to "Dimmer" at channel "ambientBrightness"
same issue openhab#9936 (but already fixed)

Signed-off-by: Michi <michael.schmidts@online.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants