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

Add support to the new Broadlink RM Mini 3 and RM4 Series #32523

Merged
merged 17 commits into from
Apr 18, 2020

Conversation

felipediel
Copy link
Contributor

@felipediel felipediel commented Mar 6, 2020

Proposed change

This PR adds support to the new RM Mini 3 (0x5f36 - Red Bean / New Black Bean) and the entire RM4 series (RM4 Mini, RM4 Pro, RM4c Mini, RM4c Pro). These devices require special headers for communication. I recently added a new class to the library in order to support these devices.

So the proposed change here is to allow the user to select the correct device type so he can control the device with Home Assistant.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

Example 1: Set up Broadlink RM Mini 3 Red Bean as a remote

# Example configuration.yaml
remote:
  - platform: broadlink
    name: Bedroom
    host: 192.168.0.12
    mac: 34-EA-34-B1-54-2C
    type: rm_mini3_redbean

Example 2: Set up Broadlink RM4 Mini as a switch

# Example configuration.yaml
switch:
  - platform: broadlink
    name: Bedroom
    host: 192.168.0.12
    mac:  34-EA-34-B1-54-2C
    type: rm4_mini
    switches:
      bedroom_tv:
        command_on: 'JgAcAB0dHB44HhweGx4cHR06HB0cHhwdHB8bHhwADQUAAAAAAAAAAAAAAAA='
        command_off: 'JgAaABweOR4bHhwdHB4dHRw6HhsdHR0dOTocAA0FAAAAAAAAAAAAAAAAAAA='

Example 3: Set up Broadlink RM4 Pro as a sensor

# Example configuration.yaml
sensor:
  - platform: broadlink
    name: Bedroom
    host: 192.168.0.12
    mac: 34-EA-34-B1-54-2C
    type: rm4_pro
    monitored_conditions:
      - 'temperature'
      - 'humidity'

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

felipediel and others added 4 commits June 15, 2019 20:00
Sync the fork to the original repository
Sync the fork to the original repository
@project-bot project-bot bot added this to Needs review in Dev Mar 6, 2020
@felipediel
Copy link
Contributor Author

I was unable to test because my device stopped working after updating Home Assistant. While I was solving the problem for others, someone created a problem for me. That's how life works.

homeassistant/components/broadlink/remote.py Outdated Show resolved Hide resolved
homeassistant/components/broadlink/remote.py Outdated Show resolved Hide resolved
homeassistant/components/broadlink/remote.py Outdated Show resolved Hide resolved
homeassistant/components/broadlink/remote.py Outdated Show resolved Hide resolved
@probot-home-assistant
Copy link

Hey there @Danielhiversen, mind taking a look at this pull request as its been labeled with a integration (broadlink) you are listed as a codeowner for? Thanks!

@project-bot project-bot bot moved this from Needs review to By Code Owner in Dev Mar 6, 2020
Co-Authored-By: springstan <46536646+springstan@users.noreply.github.com>
@ghedo
Copy link

ghedo commented Mar 6, 2020

Should this be done for broadlink/switch.py as well? Otherwise if I understand this correctly it wouldn't be possible to use https://www.home-assistant.io/integrations/broadlink/#switch with the models this adds support for.

@felipediel
Copy link
Contributor Author

@ghedo Yes.

@springstan
Copy link
Member

@felipediel please update switch.py as well as @ghedo suggested :)

@springstan springstan moved this from By Code Owner to Review in progress in Dev Mar 7, 2020
Copy link
Contributor Author

@felipediel felipediel left a comment

Choose a reason for hiding this comment

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

I made as few changes as possible just to keep things going. But I think we need to discuss another way to initialize the devices. The user should not be obligated to know the device type. A HELLO_REQUEST (broadlink.discover) would be the best option, as the device sends its type in the response. As soon as we're done here, I'll open a new issue to discuss this option. For now, letting the user choose the type will do the trick.

homeassistant/components/broadlink/switch.py Outdated Show resolved Hide resolved
@felipediel
Copy link
Contributor Author

felipediel commented Mar 8, 2020

With this last commit, PyTest started reporting an AssertionError in the homeassistant.components.withings.common.WithingsDataManager class. But I did not make any changes to this module. Something is stinking in the the tests for this class. Could it be related to the time zone?

I can undo the last commit if necessary, but I really wish I didn't have to, because it integrates the new devices into the sensor platform. I can't really see anything wrong with my code. The problem seems to be somewhere else that I haven't touched, can someone help me understand what's going on?

@ghedo @springstan @Danielhiversen Do you have any idea what is causing this error?

@felipediel
Copy link
Contributor Author

I tested the same file on the dev branch.

Command

py.test tests/components/withings/test_common.py

Output

data_manager = <homeassistant.components.withings.common.WithingsDataManager object at 0x7f6fee8b3390>

    async def test_data_manager_update_sleep_date_range(
        data_manager: WithingsDataManager,
    ) -> None:
        """Test method."""
        patch_time_zone = patch(
            "homeassistant.util.dt.DEFAULT_TIME_ZONE",
            new=dt.get_time_zone("America/Los_Angeles"),
        )
    
        with patch_time_zone:
            update_start_time = dt.now()
            await data_manager.update_sleep()
    
            call_args = data_manager.api.sleep_get.call_args_list[0][1]
            startdate = call_args.get("startdate")
            enddate = call_args.get("enddate")
    
>           assert startdate.tzname() == "PST"
E           AssertionError: assert 'PDT' == 'PST'
E             - PDT
E             + PST

tests/components/withings/test_common.py:129: AssertionError

Same assertion error, even without my code. Definitely not something I should be worrying about here. But someone should be worrying about elsewhere, because this error is preventing people to contribute.

@Danielhiversen
Copy link
Member

It is fixed #32602

@MartinHjelmare
Copy link
Member

We shouldn't merge until the new features have been tested. I'll mark this as draft. Ping me when testing is done.

@MartinHjelmare MartinHjelmare marked this pull request as draft April 17, 2020 22:44
@felipediel
Copy link
Contributor Author

felipediel commented Apr 18, 2020

@MartinHjelmare The sensors have already been tested at the library level. I'm having trouble finding users with these sensors to test my implementation in Home Assistant. But I don't see what could go wrong. The interfaces are the same. Can't we merge anyway? Users are getting impatient for this update.

@MartinHjelmare
Copy link
Member

We don't accept untested features. There should either be (unit) tests or live tests should be made.

@ghedo
Copy link

ghedo commented Apr 18, 2020

FWIW, I tested both remote and switch integrations with my RM mini 3 (the (in)famous 0x5f36 model) and they indeed work.

The model I have doesn't work at all with the current home-assistant release, so it would be great if these improvements could reach an official release so I wouldn't need to patch files manually every time there is a new home-assistant release.

@elafargue
Copy link
Contributor

Tested remote and switch on rm_mini3_redbean , all functions work well. No access to rm4 with sensor unfortunately, sorry.

Copy link
Contributor Author

@felipediel felipediel left a comment

Choose a reason for hiding this comment

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

@MartinHjelmare It will be difficult to find someone to test the sensor platform with RM 4 devices in the short term. How about we look at the tests we have so far?

homeassistant/components/broadlink/sensor.py Show resolved Hide resolved
homeassistant/components/broadlink/sensor.py Show resolved Hide resolved
@felipediel
Copy link
Contributor Author

felipediel commented Apr 18, 2020

@MartinHjelmare Everything was tested, but some tests were done separately. No problem, right?

Edit: Check this out. People are using the code without any problems. But they are being tortured with technical implementation details. They want their devices to work natively with Home Assistant. We should merge this PR as soon as possible.

@sbeff
Copy link

sbeff commented Apr 18, 2020

@felipediel Amazon will deliver the sensor cable to me today.

@felipediel
Copy link
Contributor Author

felipediel commented Apr 18, 2020

Ok, I think we need one more test. Can anyone confirm if broadlink.learn and broadlink.send are working with RM Mini 3 0x5f36 in this latest update?

@sbeff Could you test sensor platform?

@hakana
Copy link

hakana commented Apr 18, 2020

I have tested this PR with RM4 mini now. Everything (Learn, send, temperature and humidity) work as expected.

@sbeff
Copy link

sbeff commented Apr 18, 2020

@felipediel I followed the steps above, but I see this error from the logs:

Failed to connect to device

I double checked the MAC and the IP. The device works from the Broadlink iPhone app. This is my current configuration:

sensor:
  - platform: broadlink
    host: 192.168.1.16
    mac: 24-DF-A7-34-94-7F
    name: BroadLink
    type: rm4_mini
    monitored_conditions:
      - 'temperature'
      - 'humidity'

@chemelli74
Copy link
Contributor

The device works from the Broadlink iPhone app. This is my current configuration:

You don't have to configure the device in the app, this will prevent local connections.
Reset the device, add only wifi configuration and then abort app and try in HA.

Simone

@sbeff
Copy link

sbeff commented Apr 18, 2020

Thanks @chemelli74, now everything started working properly.

@felipediel felipediel marked this pull request as ready for review April 18, 2020 18:58
@rngtng
Copy link
Contributor

rngtng commented Apr 18, 2020

@felipediel with your new instructions I got your changes finally running. Tested with RM Mini 3 (0x5f36) and it works perfectly - remote learn & use as switch... great work. Thanks a lot!

@felipediel
Copy link
Contributor Author

@MartinHjelmare All tests complete. Ready to be merged.

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Good!

@MartinHjelmare
Copy link
Member

Codecov is flaky.

@MartinHjelmare MartinHjelmare merged commit a365f45 into home-assistant:dev Apr 18, 2020
Dev automation moved this from Reviewer approved to Done Apr 18, 2020
@felipediel
Copy link
Contributor Author

Thanks to everyone who contributed promptly to these final tests!
That's it, enjoy your new devices! 🚀

@userMak
Copy link

userMak commented Apr 19, 2020

sorry deleted.

@bourliam
Copy link

Hello
First thanks for the work ! You're awesome !

Do we know when this PR will be included in an update ? It's frustrating to know that the code is ready but not being able to use it !

Thanks

@sagitt
Copy link

sagitt commented Apr 23, 2020

Hello
First thanks for the work ! You're awesome !

Do we know when this PR will be included in an update ? It's frustrating to know that the code is ready but not being able to use it !

Thanks

yes. see 0.109beta release note.

@lock lock bot locked and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

Red RM Mini 3 does not work