Add async_set_datetime and async_get_datetime methods#761
Merged
bieniu merged 9 commits intobieniu:masterfrom Apr 9, 2026
Merged
Add async_set_datetime and async_get_datetime methods#761bieniu merged 9 commits intobieniu:masterfrom
bieniu merged 9 commits intobieniu:masterfrom
Conversation
Many Brother printers (especially older inkjet models like the DCP-J552DW) lose their clock after a power outage, which breaks scanning functionality. This adds SNMP-based methods to read and set the printer's date/time via the standard hrSystemDate.0 OID (1.3.6.1.2.1.25.1.2.0) using a configurable write community string (default: "internal"). New API: - Brother(write_community=...) parameter - async_get_datetime() -> datetime | None - async_set_datetime(dt=None) sets clock (defaults to local time) Made-with: Cursor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds SNMP DateAndTime support: new constants and utilities to encode/decode RFC 2579 DateAndTime, plus Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Brother
participant SNMP_Engine as SNMP Engine
participant Device
Client->>Brother: async_get_datetime()
Brother->>SNMP_Engine: get_cmd(OID_DATETIME, request_args)
SNMP_Engine->>Device: SNMP GET request
Device-->>SNMP_Engine: DateAndTime octets / error
SNMP_Engine-->>Brother: response / errindication
Brother->>Brother: parse_dateandtime(octets) or map error
Brother-->>Client: datetime | None | SnmpError | ConnectionError
sequenceDiagram
actor Client
participant Brother
participant SNMP_Engine as SNMP Engine
participant Device
Client->>Brother: async_set_datetime(dt?)
Brother->>Brother: build_dateandtime(dt or now)
Brother->>Brother: _request_args_for(write_community)
Brother->>SNMP_Engine: set_cmd(OID_DATETIME, OctetString(encoded), request_args)
SNMP_Engine->>Device: SNMP SET request
Device-->>SNMP_Engine: confirmation or error-status
SNMP_Engine-->>Brother: response / errindication
alt Success
Brother-->>Client: None
else Error
Brother-->>Client: SnmpError | ConnectionError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@brother/__init__.py`:
- Around line 382-387: The DateAndTime parser can raise ValueError for invalid
month/day/etc and should fail gracefully; in the function that parses the
DateAndTime payload (using DATEANDTIME_MIN_LENGTH and the raw byte slice and
constructing datetime(...)), validate the extracted fields or wrap the
datetime(...) call in a try/except that catches ValueError (and TypeError if
needed) and returns None on failure so Brother.async_get_datetime() never leaks
an undocumented exception; reference the code that reads year =
int.from_bytes(raw[0:2], "big") and the subsequent datetime(...) construction
and ensure invalid/zeroed month/day/hour/minute/second values are handled by
returning None.
- Line 104: The assignment uses truthiness so an empty string becomes
DEFAULT_WRITE_COMMUNITY; change it to preserve explicit empty values by only
defaulting when the argument is None: in the constructor where
self._write_community is set (symbol: self._write_community and
DEFAULT_WRITE_COMMUNITY), replace the `write_community or
DEFAULT_WRITE_COMMUNITY` logic with an explicit None check (e.g., set to
DEFAULT_WRITE_COMMUNITY only if write_community is None), leaving
blank/empty-string values intact.
- Around line 304-313: The current logic treats any errstatus as "None" which
hides real agent errors; update the error handling in the block around get_cmd
so that: if errindication is set raise SnmpError with its message; if errstatus
is set, inspect the error code/text and only return None when it indicates an
unsupported/missing OID (e.g., noSuchName/noSuchObject/noSuchInstance depending
on SNMP version); for any other errstatus raise SnmpError (include errstatus and
restable details). Refer to get_cmd, errindication, errstatus, restable, and
SnmpError to locate and modify the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07fad32d-82a0-4f46-afc9-bdc53be2e9ae
📒 Files selected for processing (3)
brother/__init__.pybrother/const.pytests/test_datetime.py
- Use explicit `is None` check for write_community to preserve empty strings - Only return None for noSuchName in async_get_datetime; raise SnmpError for other SNMP errors (e.g. genErr) instead of silently hiding them - Wrap datetime construction in _parse_dateandtime with try/except ValueError to handle malformed DateAndTime payloads from the printer gracefully - Add tests for genErr handling and invalid DateAndTime values Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
brother/__init__.py (1)
148-150: Reduce the order-dependent argument wiring.
create()now forwards seven positional constructor args, andasync_set_datetime()rebuilds_request_argsvia tuple indexes. Both are easy to break on the next signature or tuple-shape change. A small helper / keyword-based call would make this path less brittle.♻️ One way to make the forwarding explicit
- instance = cls( - host, port, community, printer_type, model, snmp_engine, write_community - ) + instance = cls( + host=host, + port=port, + community=community, + printer_type=printer_type, + model=model, + snmp_engine=snmp_engine, + write_community=write_community, + )def _request_args_for(self, community: str) -> tuple[ SnmpEngine, CommunityData, UdpTransportTarget, ContextData ]: return ( self._request_args[0], CommunityData(community, mpModel=0), self._request_args[2], self._request_args[3], )Also applies to: 354-359
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brother/__init__.py` around lines 148 - 150, The constructor-forwarding in create() and the tuple-index rebuild in async_set_datetime() are order-dependent and brittle; add a small helper method (e.g., _request_args_for(self, community: str) or build_request_args) that returns the four request args as named components (SnmpEngine, CommunityData(...), UdpTransportTarget, ContextData) instead of slicing by tuple indexes, update async_set_datetime() (and the other site around the same pattern) to call that helper, and change create() to construct the instance using explicit keyword arguments or by passing the helper-produced values so the wiring is explicit and resilient to signature/tuple-shape changes (reference symbols: create, async_set_datetime, and the new _request_args_for/build_request_args).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@brother/__init__.py`:
- Around line 148-150: The constructor-forwarding in create() and the
tuple-index rebuild in async_set_datetime() are order-dependent and brittle; add
a small helper method (e.g., _request_args_for(self, community: str) or
build_request_args) that returns the four request args as named components
(SnmpEngine, CommunityData(...), UdpTransportTarget, ContextData) instead of
slicing by tuple indexes, update async_set_datetime() (and the other site around
the same pattern) to call that helper, and change create() to construct the
instance using explicit keyword arguments or by passing the helper-produced
values so the wiring is explicit and resilient to signature/tuple-shape changes
(reference symbols: create, async_set_datetime, and the new
_request_args_for/build_request_args).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58c8c89c-8b79-4dfa-beeb-9ce5a06902a8
📒 Files selected for processing (2)
brother/__init__.pytests/test_datetime.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_datetime.py
Reduces order-dependent positional forwarding in create() and eliminates tuple-index slicing in async_set_datetime(). Made-with: Cursor
bieniu
requested changes
Mar 26, 2026
Owner
bieniu
left a comment
bieniu
left a comment
There was a problem hiding this comment.
Thanks for your PR. I didn't have time to thoroughly review the code, so here are a few comments that caught my eye.
| @@ -0,0 +1,189 @@ | |||
| """Tests for datetime get/set functionality.""" | |||
Owner
There was a problem hiding this comment.
The test file should match the code file. There is no datetime.py file in the Brother library.
| ) | ||
|
|
||
| @staticmethod | ||
| def _parse_dateandtime(raw: bytes) -> datetime | None: |
Owner
There was a problem hiding this comment.
This function should be moved to the utils module.
| ) | ||
|
|
||
| @staticmethod | ||
| def _build_dateandtime(dt: datetime) -> bytes: |
Owner
There was a problem hiding this comment.
This function should be moved to the utils module.
| HOST = "localhost" | ||
|
|
||
|
|
||
| def _setup_request_args(brother: Brother) -> None: |
- Move build_dateandtime and parse_dateandtime to utils module - Merge datetime tests into test_init.py (match code file convention) - Use pytest fixture for fake _request_args setup Made-with: Cursor
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #761 +/- ##
==========================================
+ Coverage 97.29% 97.65% +0.36%
==========================================
Files 5 5
Lines 370 427 +57
==========================================
+ Hits 360 417 +57
Misses 10 10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Format brother/__init__.py and brother/utils.py (fixes CI ruff format --check). - Add tests for async_get_datetime when get_cmd returns errindication or raises PySnmpError (improves patch coverage for Codecov). Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_init.py (1)
773-775: Avoid asserting through_ObjectType__args.Line 775 uses a private Python field, making the test brittle to PySNMP refactoring. The
ObjectTypeclass exposes a public sequence protocol (per RFC 1902) supporting tuple unpacking and index access.♻️ Use public interface
args = mock_set.call_args oid_arg = args[0][-1] - val = oid_arg._ObjectType__args[1] + _, val = oid_arg expected = b"\x07\xea\x03\x1a\x0e\x1e\x00\x00"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_init.py` around lines 773 - 775, The test is accessing a private attribute on ObjectType (oid_arg._ObjectType__args[1]) which is brittle; instead use the public sequence protocol on ObjectType (tuple-unpacking or index access). Replace the private access with something like val = tuple(oid_arg)[1] or val = oid_arg[1] after locating the variables set in tests/test_init.py (refs: mock_set.call_args, oid_arg = args[0][-1]) so the test reads the second element via the public API rather than _ObjectType__args.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@brother/__init__.py`:
- Around line 348-350: The docstring incorrectly states that an unreachable
printer raises ConnectionError; update the Raises section to document that SNMP
timeouts and errindication values are raised as SnmpError (per the errindication
handling in the code around the block that routes SNMP timeouts/errindication to
SnmpError at lines analogous to 369–373), and only mention ConnectionError if
there are other network-level connection failures actually raised elsewhere;
ensure the Raises lists SnmpError (for SNMP timeouts/errindication) and
ConnectionError only when applicable.
In `@tests/test_init.py`:
- Around line 781-790: The test test_set_datetime_default_uses_now currently
only asserts set_cmd was called; update it to freeze time (e.g., via freezegun
or monkeypatching datetime) before calling
brother_with_request_args.async_set_datetime(), then inspect the
mock_set.call_args to extract the DateAndTime octets sent by async_set_datetime
and assert they match the frozen UTC/local instant and expected timezone
encoding; ensure you still use AsyncMock for set_cmd and keep the same call but
replace the single assert_called_once() with assertions that the emitted octet
sequence equals the expected bytes derived from the frozen time and timezone.
---
Nitpick comments:
In `@tests/test_init.py`:
- Around line 773-775: The test is accessing a private attribute on ObjectType
(oid_arg._ObjectType__args[1]) which is brittle; instead use the public sequence
protocol on ObjectType (tuple-unpacking or index access). Replace the private
access with something like val = tuple(oid_arg)[1] or val = oid_arg[1] after
locating the variables set in tests/test_init.py (refs: mock_set.call_args,
oid_arg = args[0][-1]) so the test reads the second element via the public API
rather than _ObjectType__args.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de6806f1-6b1e-45f9-9ad2-5e41abee8b2c
📒 Files selected for processing (3)
brother/__init__.pybrother/utils.pytests/test_init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- brother/utils.py
- Clarify async_set_datetime Raises (SnmpError vs ConnectionError). - Strengthen test_set_datetime_default_uses_now with freeze_time, TZ=UTC, and octet assertion vs build_dateandtime(now). - Document why tests read ObjectType via _ObjectType__args (mocked varbind is not unpackable; __getitem__ raises SmiError). Made-with: Cursor
Contributor
Author
|
@bieniu I think all requested changes are now solved, please take a look |
Owner
|
The code looks good. Therefore, I'd like to ask how you plan to use these two new methods. Do you intend to implement the Brother integration change in the HA repository? If so, I'd like you to open a draft PR so we can analyze how it will work. |
Contributor
Author
|
@bieniu I can work on broader support, I'll just need few more details. If you want we can catch up on Discord: lorek123. |
bieniu
requested changes
Apr 5, 2026
| LCD.unconfigure(self._snmp_engine, None) | ||
|
|
||
| async def async_get_datetime(self) -> datetime | None: | ||
| """Read the printer's current date and time via SNMP. |
Owner
There was a problem hiding this comment.
We don't need such an extended docstring here.
| return parse_dateandtime(raw) | ||
|
|
||
| async def async_set_datetime(self, dt: datetime | None = None) -> None: | ||
| """Set the printer's date and time via SNMP. |
Owner
There was a problem hiding this comment.
We don't need such an extended docstring here.
|
|
||
| _LOGGER.debug("Printer datetime set to %s", dt.isoformat()) | ||
|
|
||
| def _request_args_for( |
| raw: bytes = restable[0][-1].asOctets() | ||
| return parse_dateandtime(raw) | ||
|
|
||
| async def async_set_datetime(self, dt: datetime | None = None) -> None: |
Owner
There was a problem hiding this comment.
We know that setting the date doesn't work on all printer models. Therefore, we need to maintain a list of printers that support this feature and raise an exception when attempting to use this method on a printer model that isn't on the list.
Contributor
Author
There was a problem hiding this comment.
Added an allowlist, do we want to have a way to test for users if their printer has this option?
| printer_type: str = "laser", | ||
| model: str | None = None, | ||
| snmp_engine: SnmpEngine | None = None, | ||
| write_community: str | None = None, |
Owner
There was a problem hiding this comment.
Suggested change
| write_community: str | None = None, | |
| write_community: str = DEFAULT_WRITE_COMMUNITY, |
- Add DATETIME_SET_SUPPORTED_MODELS allowlist (dcp-j552dw); async_set_datetime raises UnsupportedModelError for unlisted or unknown models - Change write_community param default to DEFAULT_WRITE_COMMUNITY (drop None) - Rename _request_args_for -> _write_request_args; absorb community internally - Simplify docstrings on async_get_datetime and async_set_datetime - Replace _ObjectType__args private access with OctetString patch in tests - Add tests for unsupported model and unknown model (pre-async_update) cases Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Sort DATETIME_SET_SUPPORTED_MODELS import alphabetically (I001) - Assign f-string to variable before raising UnsupportedModelError (EM102) - Shorten two test docstrings to fit within 88-char line limit (E501) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
bieniu
requested changes
Apr 7, 2026
| m in model.lower() for m in DATETIME_SET_SUPPORTED_MODELS | ||
| ): | ||
| msg = f"Setting datetime is not supported on model {model}" | ||
| raise UnsupportedModelError(msg) |
Owner
There was a problem hiding this comment.
Let's add a new exception for this purpose, e.g. MethodNotSupportedError. UnsupportedModelError doesn't suit here
Lines 17 to 18 in b5430b8
|
|
||
| async def async_set_datetime(self, dt: datetime | None = None) -> None: | ||
| """Set the printer's date and time via SNMP.""" | ||
| model = getattr(self, "model", None) |
Owner
There was a problem hiding this comment.
The printer model is available as self.model, we don't need to use getattr()
| async def async_set_datetime(self, dt: datetime | None = None) -> None: | ||
| """Set the printer's date and time via SNMP.""" | ||
| model = getattr(self, "model", None) | ||
| if model is None or not any( |
Owner
There was a problem hiding this comment.
self.model is a string, no need to check if it is None
- Add MethodNotSupportedError exception for method-level capability checks - Add is_datetime_set_supported property so callers can probe support before SET - async_set_datetime raises MethodNotSupportedError (not UnsupportedModelError) for models outside DATETIME_SET_SUPPORTED_MODELS - Use self.model directly in check; remove getattr/None guard - Update snapshots and tests accordingly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
bieniu
approved these changes
Apr 9, 2026
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
2 participants
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.
Closes #28
Summary
Many Brother printers (especially older inkjet models like the DCP-J552DW) lose their clock after a power outage, which breaks scanning functionality until the date/time is manually reset on the control panel.
This PR adds SNMP-based methods to read and set the printer's date/time via the standard
hrSystemDate.0OID (1.3.6.1.2.1.25.1.2.0), enabling automated clock restoration (e.g. via Home Assistant).New API
Brother(write_community=...)— optional parameter for the SNMP write community string (defaults to"internal", which is the standard Brother write community)async_get_datetime() -> datetime | None— reads the printer's current clock as a naive datetime (printer has no timezone concept)async_set_datetime(dt=None)— sets the printer's clock; defaults to the host's local time if no datetime is providedChanges
brother/__init__.py— addedset_cmdimport,write_communityparameter,async_get_datetime,async_set_datetime,_build_dateandtime,_parse_dateandtimebrother/const.py— addedOID_DATETIME,DATEANDTIME_MIN_LENGTH,DEFAULT_WRITE_COMMUNITYconstantstests/test_datetime.py— 14 new tests covering set/get, error paths, DateAndTime encoding/decoding, and community string configurationBackground
Tested live on a Brother DCP-J552DW. The
hrSystemDate.0OID is a standard HOST-RESOURCES-MIB OID supported across Brother models. The write community"internal"is the default Brother SNMP write community string, discovered via SNMP reconnaissance and firmware analysis ofBroSNMP.dll.Test plan
ruff checkpasses with no errorsasync_set_datetime()andasync_get_datetime()against a real DCP-J552DW printerMade with Cursor
Summary by CodeRabbit
New Features
Tests