Add support for Roomba 980 discovery#47696
Conversation
probot-home-assistant
Bot
added
integration: roomba
new-feature
small-pr
|
--edit-- |
|
Proof it worked? The integration tile didn't appear after i did a roomba reboot, however when i went through adding integration it had populate the roomba in the drop down, i selected that and just had to go press the button and submit.
Not sure why it shows R98---- as the model name |
MartinHjelmare
changed the title
|
there a couple more combos of names and different mac address formats in this https://github.com/NickWaterton/Roomba980-Python/blob/master/roomba/config_example.ini, unclear if these are madeup or real... but the details look real... are they worth adding? --update-- i wonder how many mac / name combos there are? anyhoo I think PR is ready to at least broaden discovery... |
|
You will need to modify this line as well |
bdraco
left a comment
bdraco
left a comment
There was a problem hiding this comment.
We will also need a test for a hostname that starts with roomba-
You can setup a dev env using these steps
https://developers.home-assistant.io/docs/development_environment/
|
Please add a test for this. |
|
You'll also need to run |
yes already done that in my private instance, i am still trying to debug why the discovery tile never seems to fire Also I don't have the skills to add a test for this, there is no test for the existing dhcp auto-discovery of roombas with the name iRobot for me to extend. I am thinking i should close this PR and come back around later? |
Turn on debug for logger:
default: warning
logs:
homeassistant.components.dhcp: debugUnplug and replug the roomba and watch for the dhcp discovery to make sure the OUI and hostname are correct |
I took care of adding the test for you. |
If you haven't updated your test environment after running hassfest, the new oui and hostname won't be taken into account. |
|
@bdraco thanks for the tips and hand holding, appreciate it. I had the DHCP debugging turned on last week, plus another terminal to my DHCP server logs to see when packet gets issued to either roomba (about once every 2.5 hours). Whats weird is the DHCP home assistant debugging never once threw an entry for either roomba's IP (saw ones for my netatmo and other devices) Even weirder is if I manually initiate adding of the integration it has the IP of both the roomba- and irobot roomba - so i know it found the IP's at some point. This is why i plan to strip back, re-clone my patch branch as is, revert all edits (without syncing back to github) see if can get the tile to kick in for the iRobot- in the orginal production code. Once i have that working in debug, will then intro my changes again and see if I can get it all to work. |
|
Can you post what you are seeing from the log? For august it looks like this. |
|
thanks, will do, need to wait for this fresh env to see an iRobot named DHCP packet, i reckon only about 60 to 80 mins based on my DHCP server logs (also shortened lease time to 30 mins for next time round) for netatmo i get this one line (so i know my DHCP debug is working) i backed out all my changes but i am still in my branch, waiting to see if the iRobot packet triggers anything |
|
Ok, so on a fresh clone using I get these DHCP debug events, but not once do i see an auto discovery tile for any of these. this is from this set of dhcp events ---edit--- |
|
what interesting is that the integration when added manually finds the two roombas automatically. I assume because the latest roombapy is being used which now has discovery builtin?
|
|
adding Wireshark capture of the DHCP packets, incase there is something weird about them |
|
|
|
I think the problem is that its a renewal and not a first request |
|
in a well maintained environment there will not be many new I just tried running this on the same node https://jcutrer.com/python/scapy-dhcp-listener and I note it doesn't pick up many of the ACKs from many of my devices either. tcpdump next to see if the packets are even making it up the stack.... |
Absolutely, thats why I just opened #48242 |
phew, I am glad it is not my uselessness, you have been super patient with me, thanks! |
|
Oh I see the issue (its late and i am slow) these ACKs are directed UDP packets, not broadcasts. Makes me wonder how the node will ever see it...
|
|
Ok, getting somewhere. I now know that we can generate a DHCP broadcast on the roombas (i7 and 980) on demand by pressing the clean button for 20 seconds and then releasing. (i will make sure to document this in the integrations docs once PR is closed) Here is the proof (finally) that my 980 code worked.
I am concerned that the first time i ran it I hit the bug below, but the second time I ran it (after restarting hass) it worked ok - thats the shot above. I assume this is an issue with the bootstrap of roombapy rather than anything I did? If folks agree i will check in the last change from running hassfest and then after that i think this is done? (and thanks to @bdraco for holding my hand and making the test case code!) |
Looks like a bug in roombapy. This should be good to go once you push the change with hassfest run |
i think i got this right, i looked at https://developers.home-assistant.io/docs/creating_integration_manifest/#dhcp not sure if linting will like my format and i am clueless how to test locally (i am using the hyper-v vm hass os image)
fixed casing
used a tuple to check for either name, need to prove it works in test env
fix black formatting
On branch patch-1 Your branch is ahead of 'origin/patch-1' by 1 commit. (use "git push" to publish your local commits) Changes to be committed: modified: homeassistant/components/roomba/manifest.json modified: homeassistant/generated/dhcp.py
|
Needs to be rebased I took care of restoring the json file to the previous and adding back in your change |
|
I have never done a rebase on a live fork/branch + PR |
I've taken care of it for you, but in the future, there is a guide here: https://developers.home-assistant.io/docs/development_catching_up/ |
|
For now you could pull by doing This assumes You can check with
|
|
@bdraco thanks for taking the time teaching me to fish, appreciate it. |
bdraco
added
new-feature
and removed
dependency-bump
4 participants
Proposed change
Add Roomba 980 models host name and mac to dhcp discovery in manifest.json for the roomba integration
Type of change
Example entry for
configuration.yaml:none
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: