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 Firmata Integration (attempt 2) #35591

Merged
merged 36 commits into from Jul 17, 2020
Merged

Add Firmata Integration (attempt 2) #35591

merged 36 commits into from Jul 17, 2020

Conversation

DaAwesomeP
Copy link
Contributor

@DaAwesomeP DaAwesomeP commented May 13, 2020

Proposed change

This pull adds a new Firmata integration.

  • Binary digital input and output is implemented.
  • Input is effectively done by push and callbacks instead of constantly telling the Arduino read inputs
  • This still uses YAML and not config flows, but if I am understanding the mandate correctly then this falls into the category of OK to do that with since it is not a plug-and-play solution. It is also basically impossible to automatically discover how the user wants to configure each pin.
  • This should deprecate the existing Arduino integration once I implement analog IO in a later pull. This one is async, uses push updates from the Arduino, and is more accurately named.

This is an updated continuation of an older pull (#24014).

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 configuration.yaml
firmata:
  - serial_port: /dev/serial/by-id/usb-Teensyduino_USB_Serial_358320-if00
    serial_baud_rate: 57600
    switches:
      - name: my_light
        pin_mode: OUTPUT
        pin: 4
        negate: true
      - name: my_other_output
        pin_mode: OUTPUT
        pin: 5
        negate: false
    binary_sensors:
      - name: stairs_motion
        pin_mode: PULLUP
        pin: 3

Additional information

The usage of a Git repo for a requirement is temporary but should allow for at least a code review. I am awaiting a release on the Pymata side to fix some issues with event loop handling in the library (it required an empty loop and closes the loop on failure)—my pull was accepted and the developer is doing some testing.

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 (see additional info about Git requirement)
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works. (this is a hardware integration without config flows, there are no tests) Tests have been added for the config flow

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. (see additional info about Git requirement)
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all. (see additional info about Git requirement)
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

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

@DaAwesomeP
Copy link
Contributor Author

I have fixed the Pymata dependency issue (the async issue in the Pypy version has been fixed). I have updated the checklist. I have started working on docs.

@DaAwesomeP
Copy link
Contributor Author

I have opened a pull for docs and updated the above checklist.

@DaAwesomeP
Copy link
Contributor Author

OK, I have been running the integration on my HA setup for a while now and I can successfully report zero issues!

.coveragerc Outdated Show resolved Hide resolved
@DaAwesomeP
Copy link
Contributor Author

OK, I ran python3 -m script.scaffold config_flow, but this seemed to have generated an entire config flow. I do not want it to appear in the integrations add menu in the UI. I am not sure if I ran the right script. I see it added a test file though. Is that the only file I need?

I am also not sure what I am testing here. There is no pymata-mocks, so I am unsure how I would simulate the connection. Would you mind clarifying what exactly I need to add/change?

@DaAwesomeP
Copy link
Contributor Author

DaAwesomeP commented Jul 11, 2020

If you have an example of what the tests and strings.json should look like for a YAML-only integration, that would be awesome.

@MartinHjelmare
Copy link
Member

MartinHjelmare commented Jul 11, 2020

We want to test that each entry point and exit point in the config flow behaves as expected and generates the correct result.

In our case here that should be one test for successful entry creation and one test for unsuccessful connection with abort as result.

Patch the library to avoid I/O in the tests. Follow the example code generated by the scaffold script.

Search for other config flow tests that test the import step. It's basically the same as testing the user step, just a different step name so the source will be different.

@DaAwesomeP
Copy link
Contributor Author

OK, so it also seems maybe I should be using the connection_error string since it is preferred as a builtin. However, this is an error instead of an abort., so my strings.json looks more like this:

{
    "config": {
        "error": {
            "cannot_connect": "[%key:common::config_flow::error::cannot_connect%]"
        },
        "step": {
            "import": {}
        }
    }
}

But I am confused how then to raise the error. Here is one example that confuses me:

async def async_step_import(self, user_input=None):
    """Handle the import."""
    errors = {}
    if user_input is not None:
        if self._host_port_alias_already_configured(user_input):
            return self.async_abort(reason="already_configured")
        _, errors = await self._async_validate_or_error(user_input)

        if not errors:
            title = _format_host_port_alias(user_input)
            return self.async_create_entry(title=title, data=user_input)

    return self.async_show_form(
        step_id="user", data_schema=_base_schema({}), errors=errors
    )

The error is raised using async_show_form(), but this is the import step, so how can a form be shown? Is there another way to trigger an error? I cannot find anything in docs.

@DaAwesomeP
Copy link
Contributor Author

Additionally, how can I stop the integration from appearing in the UI? Currently it is listed and clicking it show a white box that simply says "Aborted."

@MartinHjelmare
Copy link
Member

Our config flow should abort on error.

Don't set config flow to true in manifest.json to avoid the integration showing in the integration list in the GUI.

@DaAwesomeP
Copy link
Contributor Author

DaAwesomeP commented Jul 13, 2020

Wow the GitHub Actions CI starts up and completes much faster than the Azure Pipelines did. Very smooth! 👏

homeassistant/components/firmata/strings.json Outdated Show resolved Hide resolved
tests/components/firmata/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/firmata/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/firmata/test_config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/firmata/board.py Outdated Show resolved Hide resolved
tests/components/firmata/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/firmata/test_config_flow.py Outdated Show resolved Hide resolved
@DaAwesomeP
Copy link
Contributor Author

DaAwesomeP commented Jul 14, 2020

OK, I added two more tests for those two exceptions to maintain coverage. Please let me know if there are any other changes I need to make, and thank you for walking me through this process thus far!

@DaAwesomeP
Copy link
Contributor Author

OK, let me know if there is anything else that needs to be fixed!

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.

Looks good!

Dev automation moved this from Review in progress to Reviewer approved Jul 16, 2020
@balloob balloob merged commit 93919de into home-assistant:dev Jul 17, 2020
Dev automation moved this from Reviewer approved to Done Jul 17, 2020
@DaAwesomeP
Copy link
Contributor Author

🎉 🎊 Yay! This was a great learning process for me, thank you! Can't believe it has been so long since I started #24014. 😄 🏠

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants