fix: handle BleakError gracefully when reading device name characteristic - #248
Open
TheCheif wants to merge 4 commits into
Open
fix: handle BleakError gracefully when reading device name characteristic#248TheCheif wants to merge 4 commits into
TheCheif wants to merge 4 commits into
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideHandles failures when reading the Bluetooth GATT Device Name characteristic so the device remains marked as connected, adding a default device name fallback and localized error handling around the name read logic. Sequence diagram for updated get_device_name Bluetooth interactionsequenceDiagram
actor HomeAssistant
participant DelonghiDevice
participant BleakClient as _client
HomeAssistant->>DelonghiDevice: get_device_name()
activate DelonghiDevice
DelonghiDevice->>DelonghiDevice: _lock acquisition
DelonghiDevice->>DelonghiDevice: _connect()
alt NAME_CHARACTERISTIC readable
DelonghiDevice->>BleakClient: read_gatt_char(NAME_CHARACTERISTIC)
BleakClient-->>DelonghiDevice: name_bytes
DelonghiDevice->>DelonghiDevice: hostname = name_bytes.decode('utf-8')
else BleakError while reading name
DelonghiDevice->>DelonghiDevice: _LOGGER.debug(...)
DelonghiDevice->>DelonghiDevice: hostname = self.name or DEFAULT_DEVICE_NAME
end
DelonghiDevice->>BleakClient: write_gatt_char(CONTROLL_CHARACTERISTIC, DEBUG)
BleakClient-->>DelonghiDevice: ack
DelonghiDevice-->>HomeAssistant: hostname
deactivate DelonghiDevice
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Catching both
BleakErrorand genericExceptioninget_device_nameis quite broad; consider limiting this toBleakErroror a narrower set of expected errors to avoid masking unrelated issues. - The
if not self.hostnamefallback impliesself.hostnamemay be uninitialized or contain stale data; consider explicitly initializing or resettingself.hostnamebefore attempting the read so the logic is more predictable. - Logging the full exception at info level for
BleakErrormay be noisy in normal operation; consider using debug level or a more concise message to reduce log verbosity for expected failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Catching both `BleakError` and generic `Exception` in `get_device_name` is quite broad; consider limiting this to `BleakError` or a narrower set of expected errors to avoid masking unrelated issues.
- The `if not self.hostname` fallback implies `self.hostname` may be uninitialized or contain stale data; consider explicitly initializing or resetting `self.hostname` before attempting the read so the logic is more predictable.
- Logging the full exception at info level for `BleakError` may be noisy in normal operation; consider using debug level or a more concise message to reduce log verbosity for expected failures.
## Individual Comments
### Comment 1
<location path="custom_components/delonghi_primadonna/device.py" line_range="721-723" />
<code_context>
+ uuid.UUID(NAME_CHARACTERISTIC)
+ )
+ ).decode('utf-8')
+ except (BleakError, Exception) as error:
+ _LOGGER.info('Could not read NAME_CHARACTERISTIC: %s', error)
+ if not self.hostname:
+ self.hostname = self.name or "DeLonghi PrimaDonna"
await self._client.write_gatt_char(
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid catching bare Exception and narrow the error handling scope.
Catching `Exception` here will hide unexpected failures and turn them into a hostname fallback, making real bugs harder to spot. Limit this block to known Bluetooth-related exceptions (e.g., `BleakError` and any specific timeout/cancellation types you expect) and allow other exceptions to propagate.
Suggested implementation:
```python
try:
self.hostname = bytes(
await self._client.read_gatt_char(
uuid.UUID(NAME_CHARACTERISTIC)
)
).decode('utf-8')
except BleakError as error:
_LOGGER.info('Could not read NAME_CHARACTERISTIC: %s', error)
if not self.hostname:
self.hostname = self.name or "DeLonghi PrimaDonna"
await self._client.write_gatt_char(
uuid.UUID(CONTROLL_CHARACTERISTIC), bytearray(DEBUG)
)
```
If you expect other specific non-fatal Bluetooth-related exceptions (e.g., timeout or cancellation exceptions from the async framework or Bleak), you can extend the `except` clause to include only those explicit types instead of a bare `Exception`. That will keep the error handling narrow while still covering the known failure modes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The description mentions logging an info message, but the implementation uses
_LOGGER.debug; consider aligning the log level with the intended behavior so users can actually see the fallback in normal operation. - Instead of hardcoding "DeLonghi PrimaDonna" in
get_device_name, consider centralizing this default name in a constant or configuration to avoid duplication and ease future changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The description mentions logging an info message, but the implementation uses `_LOGGER.debug`; consider aligning the log level with the intended behavior so users can actually see the fallback in normal operation.
- Instead of hardcoding "DeLonghi PrimaDonna" in `get_device_name`, consider centralizing this default name in a constant or configuration to avoid duplication and ease future changes.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider downgrading the log for a non-critical
BleakErrorwhen reading the name characteristic frominfotodebugto avoid noisy logs on systems where this is expected to fail. - The explicit initialization
self.hostname = ''right before the nestedtrymay be redundant given the immediate reassignment in both the success and exception paths; you could simplify by assigning only inside those branches.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider downgrading the log for a non-critical `BleakError` when reading the name characteristic from `info` to `debug` to avoid noisy logs on systems where this is expected to fail.
- The explicit initialization `self.hostname = ''` right before the nested `try` may be redundant given the immediate reassignment in both the success and exception paths; you could simplify by assigning only inside those branches.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…n-critical BleakError
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The docstring/PR description says an info message is logged when the name read fails, but the implementation uses
_LOGGER.debug; consider aligning the log level (or description) so operators can actually notice when the fallback path is used.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The docstring/PR description says an info message is logged when the name read fails, but the implementation uses `_LOGGER.debug`; consider aligning the log level (or description) so operators can actually notice when the fallback path is used.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Author
|
@sourcery-ai dismiss |
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The PR description mentions logging an info message on failure, but the implementation uses
_LOGGER.debug; consider aligning the log level and wording so the behavior matches the documented intent. - When falling back to
self.name or DEFAULT_DEVICE_NAME, ensureself.nameis always a meaningful, non-empty string or add additional validation sohostnamecannot become an empty or unexpected value.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The PR description mentions logging an info message on failure, but the implementation uses `_LOGGER.debug`; consider aligning the log level and wording so the behavior matches the documented intent.
- When falling back to `self.name or DEFAULT_DEVICE_NAME`, ensure `self.name` is always a meaningful, non-empty string or add additional validation so `hostname` cannot become an empty or unexpected value.
## Individual Comments
### Comment 1
<location path="custom_components/delonghi_primadonna/device.py" line_range="721-723" />
<code_context>
+ uuid.UUID(NAME_CHARACTERISTIC)
+ )
+ ).decode('utf-8')
+ except BleakError as error:
+ _LOGGER.debug('Could not read NAME_CHARACTERISTIC: %s', error)
+ self.hostname = self.name or DEFAULT_DEVICE_NAME
await self._client.write_gatt_char(
</code_context>
<issue_to_address>
**suggestion:** Consider using a higher log level when the device name cannot be read.
Because this fallback changes user-visible behavior and may hide real connectivity or device issues, consider raising the log level (e.g. to `warning` or `info`) so these failures are visible in normal deployments, while still including the detailed error message.
```suggestion
except BleakError as error:
_LOGGER.warning('Could not read NAME_CHARACTERISTIC, using fallback name: %s', error)
self.hostname = self.name or DEFAULT_DEVICE_NAME
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+721
to
+723
| except BleakError as error: | ||
| _LOGGER.debug('Could not read NAME_CHARACTERISTIC: %s', error) | ||
| self.hostname = self.name or DEFAULT_DEVICE_NAME |
Contributor
There was a problem hiding this comment.
suggestion: Consider using a higher log level when the device name cannot be read.
Because this fallback changes user-visible behavior and may hide real connectivity or device issues, consider raising the log level (e.g. to warning or info) so these failures are visible in normal deployments, while still including the detailed error message.
Suggested change
| except BleakError as error: | |
| _LOGGER.debug('Could not read NAME_CHARACTERISTIC: %s', error) | |
| self.hostname = self.name or DEFAULT_DEVICE_NAME | |
| except BleakError as error: | |
| _LOGGER.warning('Could not read NAME_CHARACTERISTIC, using fallback name: %s', error) | |
| self.hostname = self.name or DEFAULT_DEVICE_NAME |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On some Bluetooth adapters, operating systems, or specific models of the DeLonghi PrimaDonna, the standard GATT Device Name characteristic
(
00002A00-0000-1000-8000-00805F9B34FB) is either not readable, not exposed, or blocked by the host controller.Currently, if
read_gatt_charthrows aBleakErrorwhile attempting to read this characteristic inget_device_name(), the exception is caughtby the outer block which sets
self.connected = False. This results in the integration constantly flagging itself as disconnected and looping setupattempts, despite successfully connecting and being able to write to the control characteristic.
Solution
This PR wraps the standard GATT name retrieval in a localized
try/exceptblock. If reading the name fails:self.name) or a default string soself.hostnameis populated.self.connected = True.Summary by Sourcery
Handle failures when reading the Bluetooth GATT device name without marking the device as disconnected.
Bug Fixes:
Enhancements: