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.

Sign up for GitHub

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

Jump to bottom

Update onewire component #31419

Merged
merged 7 commits into from
Feb 13, 2020
Merged

Update onewire component #31419

merged 7 commits into from
Feb 13, 2020

Conversation

MrDadoo
Copy link
Contributor

@MrDadoo MrDadoo commented Feb 2, 2020 •
edited by springstan
Loading

Proposed change

For family 26, there are a couple of other measurable values that wherer not previously retreived. This PR includes grabbing the voltages VDD, VAD and the current IAD from the device.

I also added two attributes for each sensor that might be of interest for the user.
Device file name (part of owfs path) and the raw_value that is not rounded or truncated. Example.
device_file: /28.FF5C68521604/temperature
raw_value: 28.3125

Some debug outputs were added to simplify development and fault finding.

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

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.

  • Added documentation Update onewire integration docs home-assistant.io#12071

The integration reached or maintains the following Integration Quality Scale:

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

@MrDadoo
Added some measurement points for Family 26.
893bbd3 
Added three attriubtes for each sensor, DEVICE_FILE, RAW_VALUE and SENSOR_TYPE
Added some comments.
Updated to get config values for owserver to get owport and owhost.
@homeassistant
Copy link
Contributor

Hi @MrDadoo,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@project-bot project-bot bot added this to Needs review in Dev Feb 2, 2020
@probot-home-assistant
Copy link

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

MartinHjelmare
MartinHjelmare requested changes Feb 2, 2020
View reviewed changes
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.

I suggest reverting all changes except the device sensor and sensor type constant additions.

homeassistant/components/onewire/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/onewire/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/onewire/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/onewire/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/onewire/sensor.py Outdated Show resolved Hide resolved
Dev automation moved this from Needs review to Review in progress Feb 2, 2020
@MrDadoo
Copy link
Contributor Author

MrDadoo commented Feb 3, 2020

I now think I have resolve all comments.
I am completely new to GitHub and git so I've struggled a bit with some bad commits.
Though it seems that i have a bad CLA flag on this pull request. This is because I had wrong mail specified on one of the commits. I've rebased and ammended and both my local and my GitHub hosted repo have correct username and email. Still the PR flags that somethig is invalid.

@home-assistant home-assistant deleted a comment from homeassistant Feb 4, 2020
@home-assistant home-assistant deleted a comment from homeassistant Feb 4, 2020
@home-assistant home-assistant deleted a comment from homeassistant Feb 4, 2020
@home-assistant home-assistant deleted a comment from homeassistant Feb 4, 2020
@MartinHjelmare
Copy link
Member

MartinHjelmare commented Feb 4, 2020 •
edited
Loading

There's still one commit with the unlinked email. See the bot message above.

@project-bot project-bot bot moved this from Review in progress to Needs review in Dev Feb 4, 2020
@MrDadoo
Copy link
Contributor Author

MrDadoo commented Feb 5, 2020

Now I've managed to get rid of the faulty mail.
The cla-signed label have been added.
I guess someone have to manually remove the cla-error label.

@home-assistant home-assistant deleted a comment from homeassistant Feb 5, 2020
@home-assistant home-assistant deleted a comment from homeassistant Feb 5, 2020
@MartinHjelmare MartinHjelmare changed the title Updates for onewire component Update onewire component Feb 6, 2020
@MrDadoo
Changed to let device_state_attributes return attribute device_file f…
0f3d551 
…rom the self._device_file member variable of the entity.
@MrDadoo
Copy link
Contributor Author

MrDadoo commented Feb 10, 2020

This time tests failed without any significant changes have been made.
I do not understand the test results and what is being claimed as failed.

@MartinHjelmare
Copy link
Member

The test failure is unrelated to the changes here. Don't worry about it.

Please change to debug logging per above, then we can merge.

MartinHjelmare
MartinHjelmare approved these changes Feb 10, 2020
View reviewed changes
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!

Dev automation moved this from Needs review to Reviewer approved Feb 10, 2020
@springstan
Copy link
Member

springstan commented Feb 10, 2020 •
edited
Loading

@MrDadoo please update the documentation for this integration accordingly by opening a PR in home-assistant/home-assistant.io :)

@MrDadoo
Copy link
Contributor Author

MrDadoo commented Feb 10, 2020

Please advise me where to start/find/continue documentation.

@springstan
Copy link
Member

Here is a guide on how to contribute to the home-assistant.io documentation.

@MrDadoo
Copy link
Contributor Author

MrDadoo commented Feb 12, 2020

Here is a guide on how to contribute to the home-assistant.io documentation.

I've found documents here
https://www.home-assistant.io/integrations/onewire/

I can add some lines to describe my update. But do you want me to update in directly in git, in current or next branch, or create a pull request from a clone?

I also see that the document searching on the home-assistant page finds the term "1-wire", not "onewire". Is there a way to add some kind of search tags so that integration documents can be found for both terms ? Even "ofws" would be a decent search term.

@springstan
Copy link
Member

I've found documents here
https://www.home-assistant.io/integrations/onewire/

That is the right one! You can edit this page by clicking on Edit this page on GitHub.

I can add some lines to describe my update. But do you want me to update in directly in git, in current or next branch, or create a pull request from a clone?

Yes, please add some lines describing your update. It is up to you whether or not you want do it on Github directly or via git. You will need to create a PR nevertheless. Please use the next branch since you are adding a feature to HA, see the developer documentation for reference:

Create a Pull Request (PR) against the next branch of home-assistant.io if your documentation is a new feature, platform, or integration.
Create a Pull Request (PR) against the current branch of home-assistant.io if you fix stuff, create Cookbook entries, or expand existing documentation.

To answer your last question:

I also see that the document searching on the home-assistant page finds the term "1-wire", not "onewire". Is there a way to add some kind of search tags so that integration documents can be found for both terms ? Even "ofws" would be a decent search term.

Please take a look at this issue home-assistant/home-assistant.io#11682

@MrDadoo MrDadoo mentioned this pull request Feb 12, 2020
Merged
7 tasks
@MrDadoo
Copy link
Contributor Author

MrDadoo commented Feb 13, 2020

Am I supposed to do something else with these pull requests?

@MartinHjelmare
Copy link
Member

I think we're good.

@MartinHjelmare MartinHjelmare merged commit fdfe885 into home-assistant:dev Feb 13, 2020
Dev automation moved this from Reviewer approved to Done Feb 13, 2020
@lock lock bot locked and limited conversation to collaborators Feb 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

Assignees
No one assigned
Labels
cla-signed integration: onewire
Projects
Dev
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@MrDadoo @homeassistant @MartinHjelmare @springstan