Skip to content

Support custom baud speed#68320

Merged
frenck merged 32 commits intohome-assistant:devfrom
ocalvo:SupportBaudRate
May 9, 2022
Merged

Support custom baud speed#68320
frenck merged 32 commits intohome-assistant:devfrom
ocalvo:SupportBaudRate

Conversation

@ocalvo
Copy link
Contributor

@ocalvo ocalvo commented Mar 18, 2022

Breaking change: no

Proposed change

    1. Some modems do not auto configure the communication baud speed. For these these modems, this change adds an override as part of the modem confirmation.
    1. Deprecate yaml configuration.

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

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.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

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

To help with the load of incoming pull requests:

@ocalvo ocalvo requested a review from frenck March 19, 2022 19:48
@frenck frenck added the smash Indicator this PR is close to finish for merging or closing label Mar 27, 2022
@frenck frenck removed the smash Indicator this PR is close to finish for merging or closing label Mar 30, 2022
ocalvo and others added 3 commits March 30, 2022 09:44
Co-authored-by: Franck Nijhof <frenck@frenck.nl>
Co-authored-by: Franck Nijhof <frenck@frenck.nl>
Co-authored-by: Franck Nijhof <frenck@frenck.nl>
@ocalvo ocalvo requested a review from frenck March 30, 2022 16:46
@frenck frenck removed their request for review March 30, 2022 16:48
@frenck frenck self-requested a review April 1, 2022 13:58
@ocalvo
Copy link
Contributor Author

ocalvo commented Apr 6, 2022

keep alive

1 similar comment
@ocalvo
Copy link
Contributor Author

ocalvo commented Apr 14, 2022

keep alive

@ocalvo
Copy link
Contributor Author

ocalvo commented Apr 23, 2022

Keep alive

@ocalvo
Copy link
Contributor Author

ocalvo commented Apr 23, 2022

@frenck Can you merge this?

@ocalvo ocalvo mentioned this pull request Apr 23, 2022
22 tasks
import voluptuous as vol
import logging

import voluptuous as vol # pylint: disable=import-error
Copy link
Member

Choose a reason for hiding this comment

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

Why is this causing an import error that needs to be disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems pylint does not know about the gammu lib for some reason.

Copy link
Member

@frenck frenck Apr 27, 2022

Choose a reason for hiding this comment

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

This comment is made on de Voluptuous import, not the gammu library

Copy link
Contributor Author

@ocalvo ocalvo Apr 27, 2022

Choose a reason for hiding this comment

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

Ahh I remember now. Pylint entered an infinite loop complaining about wrong import order. X before Y.
When trying to fix it then it complain than Y before X as so on.
The only way I was able to fix it was to ignore the import.
I should have added a comment.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds incorrect or maybe an issue in your environment. We don't use pylint for import ordering, as a matter of fact, the order is disabled:

CleanShot 2022-04-27 at 19 00 14

The error you are disabling in the comment is an import-error, which should not occur for voluptuous

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps you are correct.
My dev environment is based on WSL and by following the handbook I see a failure in scripts/setup complaining about a bad parameter for pip. I ask in the dev community channel but got no solutions.

I workaround this by removing the parameter which then problaby corrupted my pylint.

I will remove all of these in my real Ubuntu dev box and see if I get better results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps you are correct. My dev environment is based on WSL and by following the handbook I see a failure in scripts/setup complaining about a bad parameter for pip. I ask in the dev community channel but got no solutions.

I workaround this by removing the parameter which then problaby corrupted my pylint.

I will remove all of these in my real Ubuntu dev box and see if I get better results.

Resolved by using docker container.


import gammu # pylint: disable=import-error
import voluptuous as vol
import voluptuous as vol # pylint: disable=wrong-import-order, import-error
Copy link
Member

Choose a reason for hiding this comment

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

Why are these disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why are these disabled?

Resolved

from .const import DOMAIN
from homeassistant.helpers import selector

from .const import ( # pylint: disable=unused-import
Copy link
Member

Choose a reason for hiding this comment

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

All of them are used? Why is this pylint check disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of them are used? Why is this pylint check disabled?

Resolved

@ocalvo ocalvo requested a review from frenck April 29, 2022 17:00
@ocalvo
Copy link
Contributor Author

ocalvo commented Apr 29, 2022

After git pull origin I am seeing this error:

rror: .github/workflows/ci.yaml (Line: 564, Col: 14):
Error: The template is not valid. .github/workflows/ci.yaml (Line: 564, Col: 14): An error occurred trying to start process '/home/runner/runners/[2](https://github.com/home-assistant/core/runs/6232049747?check_suite_focus=true#step:4:2).290.1/externals/node16/bin/node' with working directory '/__w/core/core'. No such file or directory

This is most likely not related to this change.

@ocalvo
Copy link
Contributor Author

ocalvo commented May 3, 2022

Keep alive

Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Thanks, @ocalvo 👍

@frenck frenck merged commit 1cc9800 into home-assistant:dev May 9, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 10, 2022
@ocalvo ocalvo deleted the SupportBaudRate branch April 22, 2023 00:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants