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

Adjust logging level in ModBus #128980

Merged
merged 1 commit into from
Oct 23, 2024
Merged

Adjust logging level in ModBus #128980

merged 1 commit into from
Oct 23, 2024

Conversation

crug80
Copy link
Contributor

@crug80 crug80 commented Oct 22, 2024

Breaking change

Proposed change

ModBus messages related to Opening/Closing operations over the comunication channel are noew reported as info, not warning.

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)
  • Deprecation (breaking change to happen in the future)
  • 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
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format 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.

To help with the load of incoming pull requests:

@epenet epenet changed the title Fix issue 127570 in ModBus Component Adjust logging level in ModBus Oct 22, 2024
epenet
epenet approved these changes Oct 22, 2024
View reviewed changes
Copy link
Contributor

@epenet epenet left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jbouwh
Copy link
Contributor

jbouwh commented Oct 23, 2024 •
edited
Loading

@epenet Not sure we allow info log messages for non core integrations.

https://developers.home-assistant.io/docs/development_guidelines?_highlight=log#log-messages

@epenet
Copy link
Contributor

epenet commented Oct 23, 2024

@epenet Not sure we allow info log messages for non core integrations.

Yes we do (see review above and #125939 (comment))

jbouwh
jbouwh previously requested changes Oct 23, 2024
View reviewed changes
Copy link
Contributor

@jbouwh jbouwh left a comment

Choose a reason for hiding this comment

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

I think we should use debug logging instead.

homeassistant/components/modbus/modbus.py
@@ -316,7 +316,7 @@ async def async_pb_connect(self) -> None:
self._log_error(err, error_state=False)
return
message = f"modbus {self.name} communication open"
_LOGGER.warning(message)
_LOGGER.info(message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_LOGGER.info(message)
_LOGGER.debug(message)

homeassistant/components/modbus/modbus.py
@@ -368,7 +368,7 @@ async def async_close(self) -> None:
del self._client
self._client = None
message = f"modbus {self.name} communication closed"
_LOGGER.warning(message)
_LOGGER.info(message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_LOGGER.info(message)
_LOGGER.debug(message)

@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft October 23, 2024 08:32
@epenet
Copy link
Contributor

epenet commented Oct 23, 2024 •
edited
Loading

@jbouwh what do you make of this comment?

Update quality scale docs to say disconnect / reconnect should be logged at info level

And https://github.com/home-assistant/developers.home-assistant/pull/2358/files

@jbouwh
Copy link
Contributor

jbouwh commented Oct 23, 2024 •
edited by epenet
Loading

@jbouwh what do you make of this comment?

Update quality scale docs to say disconnect / reconnect should be logged at info level

And https://github.com/home-assistant/developers.home-assistant/pull/2358/files

Guess I missed this update. But it seems the policy has changed. But the is log targetting the user?

@jbouwh jbouwh dismissed their stale review October 23, 2024 08:50

Info messages are allowed when targetted to the user

@jbouwh jbouwh marked this pull request as ready for review October 23, 2024 08:53
jbouwh
jbouwh approved these changes Oct 23, 2024
View reviewed changes
Copy link
Contributor

@jbouwh jbouwh left a comment

Choose a reason for hiding this comment

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

As discussed on Discord, these messages should be logged as Info.

@jbouwh jbouwh merged commit bf8c345 into home-assistant:dev Oct 23, 2024
46 checks passed
@crug80 crug80 deleted the issue127570 branch October 23, 2024 18:03
zxdavb pushed a commit to zxdavb/hass that referenced this pull request Oct 24, 2024
@crug80 @zxdavb
Adjust logging level in ModBus (home-assistant#128980)
857c053 
Fix issue 127570 in ModBus Component
zxdavb added a commit to zxdavb/hass that referenced this pull request Oct 24, 2024
@zxdavb
dhw is now a constant
0a8368a 
Use new reauth helpers in weatherflow_cloud (home-assistant#128821)

Use new reauth helpers in wallbox (home-assistant#128820)

Simplify custom component loading (home-assistant#128813)

Use new reauth helpers in webostv (home-assistant#128823)

Use new reauth helpers in whirlpool (home-assistant#128825)

Handle invalid zeroconf messages in Android TV Remote (home-assistant#128819)

Use new reauth helpers in yale (home-assistant#128828)

Update zhong-hong-hvac to 1.0.13 (home-assistant#128822)

Use new reauth helpers in vicare (home-assistant#128779)

Auto lower case username for Schlage auth flows (home-assistant#128730)

Bump plugwise to v1.4.3 (home-assistant#128773)

Use new reauth helpers in weheat (home-assistant#128824)

Use new reauth helpers in youtube (home-assistant#128835)

Use new reauth helpers in yolink (home-assistant#128834)

Update attrs to 24.2.0 (home-assistant#126656)

Add Spotify to strict typing (home-assistant#128846)

Use new reauth helpers in yalexs_ble (home-assistant#128831)

Use new reauth helpers in withings (home-assistant#128826)

Add New Music Category for Media Browser (home-assistant#128147)

Align consumption sensor names in ViCare integration (home-assistant#127888)

Reduce the size of the Nest event media storage cache (home-assistant#128855)

Reduce max media items per nest device

Add humidity to KNX climate (home-assistant#128844)

Use new reauth helpers in yale_smart_alarm (home-assistant#128836)

Bump google-nest-sdm to 6.1.3 (home-assistant#128871)

Bump pyTibber to 0.30.3 (home-assistant#128860)

Remove myself from roomba codeowners (home-assistant#128858)

Bump habluetooth to 3.6.0 (home-assistant#128815)

Add audio feature sensors to Spotify (home-assistant#128785)

Improve entity cached attributes (home-assistant#128876)

Use runtime_data for Swiss Public Transport (home-assistant#128369)

* use runtime_data instead of hass.data[<key>]

* fix service response export type

* reduce runtime_data to be just the coordinator

* fix rebase

* fix ruff

* address reviews

* address reviews

* no general core import

* no general config_entries import

* fix also for services

* remove untyped config entry

* remove unneeded cast

Add translations for Netatmo thermostat preset modes (home-assistant#128890)

Simplify Swiss public transport coordinator (home-assistant#128891)

Include Z-Wave JS lowSecurityReason in node added websocket message (home-assistant#128896)

* Propagate lowSecurityReason to FE when adding a zwavejs device insecurely

* update tests

Remove dead code from concord232 (home-assistant#128907)

Add reconfigure flow to ring integration (home-assistant#128357)

Co-authored-by: Christopher Fenner <[email protected]>
Co-authored-by: Joost Lekkerkerker <[email protected]>

Push real binary sensor states to state machine in tests (home-assistant#128894)

Use STATE_ON/STATE_OFF constants in template test (home-assistant#128883)

Bump pyopenweathermap to v0.2.1 (home-assistant#128892)

Add ecobee set_sensors_used_in_climate service (home-assistant#102871)

* Add set_active_sensors Service

* Remove version bump from service addition commit

* Reviewer suggested changes

* Changed naming to be more clear of functionality

* Adjusted additional naming to follow new convention

* Updated to pass failing CI tests

* Fix typo

* Fix to pass CI

* Changed argument from climate_name to preset_mode and changed service error

* Made loop more clear and changed raised error to log msg

* Fix typo

Co-authored-by: Erik Montnemery <[email protected]>

* Removed code that was accidentally added back in and fixed mypy errors

* Add icon for service

* Added sensors as attributes and updated tests

* Revert changes made in home-assistant#126587

* Added tests for remote_sensors and set_sensors_used_in_climate

* Changed back to load multiplatforms (home-assistant#126587)

* Check for empty sensor list and negative tests for errors raised

* Added tests and fixed errors

* Add hass to class init to allow for device_registry lookup at startup and check for name changed by user

* Added tests to test the new functions

* Simplified code and fixed testing error for simplification

* Added freeze in test

* Fixed device filtering

* Simplified code section

* Maintains the ability to call `set_sensors_used_in_climate` function even is the user changes the device name from the ecobee app or thermostat without needing to reload home assistant.

* Update tests with new functionality. Changed thermostat identifier to a string, since that is what is provided via the ecobee api

* Changed function parameter

* Search for specific ecobee identifier

* Moved errors to strings.json

* Added test for sensor not on thermostat

* Improved tests and updated device check

* Added attributes to _unrecoreded_attributes

* Changed name to be more clear

* Improve error message and add test for added property

* Renamed variables for clarity

* Added device_id to available_sensors to make it easier on user to find it

---------

Co-authored-by: Robert Resch <[email protected]>
Co-authored-by: Erik Montnemery <[email protected]>

Add Airzone switch entities to zones (home-assistant#124562)

Add new QNAP QSW uptime timestamp sensor (home-assistant#122589)

Co-authored-by: Joost Lekkerkerker <[email protected]>

Remove explicit templating of persistent_notification service data (home-assistant#128903)

Add Airzone Cloud main zone mode select (home-assistant#125918)

Co-authored-by: Joost Lekkerkerker <[email protected]>

Add Airzone Cloud switch entities to zones (home-assistant#125917)

Co-authored-by: Joost Lekkerkerker <[email protected]>

Add floor heating device valve positions in Homematic IP Cloud  (home-assistant#122759)

* Update sensor.py for new FALMOT Sensors

First Integration attemp to support ValvePosition as Sensor for HmIP-FALMOT-C12

* Update sensor.py

* Update sensor.py

* Add Valve Position to FALMOT-C12

* modified: devcontainer

* Service für minimum vale postion hinzugefügt.

* update to services

* Service call optimized

* Add valvePosition to HomematicIP Cloud for Falmot-C12 and show only channels that are connected with an motorized actuator

* Fix some tests

* Add icon for service

* Fix tests, add check for ValveState in icon

* Remove minimum valve service

* REmove minimum valve

* Use list comprehension for devices, support other terminal blocks

* Remove unused constant

* Check correct channel

---------

Co-authored-by: thecem <[email protected]>

Add fan `set_speed` support for Xiaomi Mi Air Purifier 3C (home-assistant#126870)

Add config flow to local_file (home-assistant#125835)

* Add config flow to local_file

* Small mods

* Add/fix tests

* Fix

* slug

* Fix strings

* Mod strings

Add firmware update entity to IronOS integration (home-assistant#123031)

Add diagnostics to Comelit SimpleHome (home-assistant#128794)

* Add diagnostics to Comelit SimpleHome

* add test

* add missing tests

* introduce SnapshotAssertion

* cleanup

* exclude date based props

Deprecate entity_id template variable in camera services (home-assistant#128592)

* Deprecate entity_id template variable in camera services

* Update snapshots

* Tiny lang tweak

* Fix translation

---------

Co-authored-by: Franck Nijhof <[email protected]>

Add diagnostics to Vodafone Station (home-assistant#128923)

* Add diagnostics to Vodafone Station

* cleanup and exclude props based on date

Add update_percentage property to update entity (home-assistant#128908)

Allow Trend title to be translated (home-assistant#128926)

Fix description placeholder in fibaro reauth (home-assistant#128925)

Allow Random title to be translated (home-assistant#128928)

Bump holidays to 0.59 (home-assistant#128924)

Remove explicit templating of telegram_bot service data (home-assistant#128906)

Remove explicit templating of minio service data (home-assistant#128905)

Remove explicit templating of velbus service data (home-assistant#128904)

Remove explicit templating of logbook service data (home-assistant#128902)

Allow Timer title to be translated (home-assistant#128927)

Fix description placeholder in brunt reauth (home-assistant#128933)

* Fix description placeholder in brunt reauth

* Update homeassistant/components/brunt/config_flow.py

Co-authored-by: Jan-Philipp Benecke <[email protected]>

* Update homeassistant/components/brunt/config_flow.py

Co-authored-by: Jan-Philipp Benecke <[email protected]>

---------

Co-authored-by: Jan-Philipp Benecke <[email protected]>

Add subscription tier attribute to Twitch integration. (home-assistant#128870)

* Add subscription tier to Twitch integration.

* Add test for Twitch tiers.  Tests do not currently pass, so this is only theoretical.

* Fix variable type

* Show tier levels as 1,2,3 instead of the raw API values of 1000,2000,3000.

* Make Twitch subscription tier fixtures strings.

* Use proper assertion value for subscription tier test.

Edited on a bus on my phone. 😎

* Update homeassistant/components/twitch/coordinator.py

* Update tests/components/twitch/test_sensor.py

---------

Co-authored-by: Joost Lekkerkerker <[email protected]>

Add missing strings for mold indicator (home-assistant#128205)

* Add missing localization keys for random component configuration

* Add missing localization keys for mold_indicator component configuration

* one_integration_at_a_time

* Fix localization strings for mold_indicator: use direct values instead of non-existing keys

* Fix localization strings for mold_indicator: use direct values instead of non-existing key

* Add missing translations for Mold Indicator helper

* correcting it for hassfest

* Fixes

---------

Co-authored-by: G Johansson <[email protected]>

Drop not needed reauth strings in tplink (home-assistant#128937)

Use new reauth helpers in unifi (home-assistant#128837)

* Use new reauth helpers in unifi

* Apply suggestions from code review

Co-authored-by: Joost Lekkerkerker <[email protected]>

* Update config_flow.py

---------

Co-authored-by: Joost Lekkerkerker <[email protected]>

Fix description placeholder in imap reauth (home-assistant#128940)

Implement new state property for alarm_control_panel which is using an enum (home-assistant#126283)

* Alarm state from enum

* Fixes

* Set final

* Fix rebase

* Test const

* Fix breaking version

* Fix other for alarm_control_panel

* Fix integrations

* More

* More

* More

* More

* Fix zha

* Replace _attr_state

* Fix alarm_control_panel

* Fix tests

* Fixes

* Mods

* Change some

* More

* More

* More

* Tests

* Last tests

* Return enum

* Fix zha

* Remove not needed check

* Fix wording

* Fix homekit

* Mod prometheus

* Fix mypy

* Fix homekit

* Fix ifttt

Fix description placeholder in transmission reauth (home-assistant#128938)

Add motion detected binary_sensor for tplink (home-assistant#127883)

* Add motion binary_sensor for tplink

* Remove strings definition as we have device class that handles this

* Simplify instructions

* Remove mentions about fixture creation and snapshot updates as requested

* re-add newline

Expose tplink temperature sensor as measurement (home-assistant#128640)

Add state_class=measurement to the temperature sensor, making it available for long-term statistics.

Fix flaky update coordinator test (home-assistant#128943)

Bump xiaomi-ble to 0.33.0 (home-assistant#128946)

Update astroid to 3.3.5 (home-assistant#128948)

Bump yarl to 1.16.0 (home-assistant#128941)

Bump gcal_sync to 6.2.0 (home-assistant#128949)

Add snapshot service to image entity (home-assistant#110057)

* Add service definition for saving snapshot of image entity

* Add service to image

* Add tests for image entity service

* Fix tests

* Formatting

* Add service icon

* Formatting

* Formatting

* Raise home assistant error instead of single log error

* Correctly pass entity id

* Raise exception from existing exception

* Expect home assistant error

* Fix services example

* Add test for templated snapshot

* Correct icon service config

* Set correct type for service template

* Remove unneeded

Co-authored-by: Erik Montnemery <[email protected]>

* remove template

* fix imports

* Update homeassistant/components/image/__init__.py

* Apply suggestions from code review

---------

Co-authored-by: Erik Montnemery <[email protected]>

Add OSO Energy services (home-assistant#118770)

* Add OSO Energy services

* Fixes after review

* Add tests for OSO Energy water heater

* Fixes after review

* Revert changes for service schema in OSO Energy

* Improve osoenergy unit tests

Fix google tasks todo docstrings (home-assistant#128978)

Add support for fetching bindkey from Mi cloud (home-assistant#128394)

Fix zha test RuntimeWarnings (home-assistant#128975)

Bump aiocomelit to 0.9.1 (home-assistant#128977)

* Bump aiocomelit to 0.9.1

* remove exception

Bump aiovodafone to 0.6.1 (home-assistant#128976)

* Bump aiovodafone to 0.6.1

* remove exception

Bump PySwitchBot to 0.51.0 (home-assistant#128990)

Add limited template to at field for time triggers (home-assistant#126584)

* Add limited template to at field for time triggers

* fix mypy

* Fix comments

* fix-tests

---------

Co-authored-by: Erik Montnemery <[email protected]>

Update aioairzone-cloud to v0.6.8 (home-assistant#128992)

Bump axis to v63 (home-assistant#129005)

Bump python-roborock to 2.6.1 (home-assistant#128804)

Bump lektricowifi to 0.0.43 (home-assistant#128979)

Use ConfigEntry.runtime_data in gardena_bluetooth (home-assistant#129000)

Improve template docstring (home-assistant#128967)

Fix step in presets for generic thermostat (home-assistant#128922)

Expose scripts with no fields as entities (home-assistant#123061)

Fix FUNDING.yml to OHF (home-assistant#129013)

Add Hassio HTTP logs/follow to allowed paths (home-assistant#126606)

* Add logs/follow to admin paths in hassio.http

* Add tests for logs/follow admin paths in hassio.http

* Add tests for logs/follow admin paths in hassio.http

* Add compress and timeout exclusions for hassio http api

* Fix should_compress usage in hassio/ingress

* Add missing follow exceptions for hassio/http

* Add hassio range header forward for logs endpoints

* Fix test syntax hassio/http

Bump orjson to 3.10.10 (home-assistant#129015)

changelog: ijl/orjson@3.10.9...3.10.10

Adjust logging level in ModBus (home-assistant#128980)

Fix issue 127570 in ModBus Component

Remove battery device class from bmw secondary sensor (home-assistant#128970)

Remove battery device class

Add go2rtc binary config to expose api only on localhost (home-assistant#129025)

Bump github/codeql-action from 3.26.13 to 3.27.0 (home-assistant#129019)

Bumps [github/codeql-action](https://github.com/github/codeql-action) from 3.26.13 to 3.27.0.
- [Release notes](https://github.com/github/codeql-action/releases)
- [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md)
- [Commits](github/codeql-action@v3.26.13...v3.27.0)

---
updated-dependencies:
- dependency-name: github/codeql-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Powerview migrate scene to string unique_id (home-assistant#128131)

Bump pyduotecno to 2024.10.1 (home-assistant#128968)

Bump python bsblan version 0.6.4 (home-assistant#128999)

Allow configuring WebRTC stun and turn servers (home-assistant#128984)

* Allow configuring WebRTC stun and turn servers

* Add tests

* Remove class WebRTCCoreConfiguration

Have statistics functions return a meaningful, non-none result even if only one value is available (home-assistant#127305)

* have statistics functions return a meaningful, non-none result even if only one value is available

* improved code coverage

Add switch platform to the Lektrico integration (home-assistant#126721)

Fix devolo_home_network devices not reporting a MAC address (home-assistant#129021)

Bump actions/cache from 4.1.1 to 4.1.2 (home-assistant#129018)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Remove deprecated channel views attribute from Twitch (home-assistant#129008)

Use runtime_data in balboa (home-assistant#129035)

Add `completed` to the wait variable when using triggers (`wait_for_trigger`) (home-assistant#123427)

* Add support for the wait.completed variable when using wait with triggers

* Remove junk comment

---------

Co-authored-by: Erik Montnemery <[email protected]>

Use runtime_data in bang_olufsen (home-assistant#129037)

Create tests for sense integration (home-assistant#128418)

* Create tests for sense integration

* Rearrange files

* Update to use snapshots

* Update tests/components/sense/__init__.py

Co-authored-by: epenet <[email protected]>

* Update tests/components/sense/__init__.py

Co-authored-by: epenet <[email protected]>

* Update tests/components/sense/test_binary_sensor.py

Co-authored-by: epenet <[email protected]>

* Update tests/components/sense/test_sensor.py

Co-authored-by: epenet <[email protected]>

* Add missing imports

---------

Co-authored-by: epenet <[email protected]>

Bump yt-dlp to 2024.10.22 (home-assistant#129034)

Bump sensorpush-ble to 1.7.0 (home-assistant#128951)

changelog: Bluetooth-Devices/sensorpush-ble@v1.6.2...v1.7.0

Fix calculation of attributes in group sensor (home-assistant#128601)

* Fix calculation of attributes in group sensor

* Fixes

* Fixes

* Make module level function

Fix get_time_zone annotations in dt_util (home-assistant#129050)

Fix cancellation leaking upward from the timeout util (home-assistant#129003)
@github-actions github-actions bot locked and limited conversation to collaborators Oct 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jbouwh jbouwh jbouwh approved these changes

@epenet epenet epenet approved these changes

Assignees
No one assigned
Labels
bugfix cla-signed Hacktoberfest integration: modbus Quality Scale: silver small-pr PRs with less than 30 lines.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Warning: modbus communication open on startup
3 participants
@crug80 @jbouwh @epenet