Skip to content

classic LSS scan#298

Open
LBekel wants to merge 1 commit intocanopen-python:masterfrom
LBekel:LSS_ident_remote_slave
Open

classic LSS scan#298
LBekel wants to merge 1 commit intocanopen-python:masterfrom
LBekel:LSS_ident_remote_slave

Conversation

@LBekel
Copy link
Copy Markdown

@LBekel LBekel commented Mar 8, 2022

added return value to send_identify_remote_slave function to perform classic LSS scan, add flag to remove delay from __send_lss_address function

…classic LSS scan, add flag to remove delay from __send_lss_address function
@bizfsc
Copy link
Copy Markdown
Collaborator

bizfsc commented May 4, 2026

Thanks for this PR — the basic idea is correct and follows the existing pattern from send_switch_state_selective. However, there are a few issues that need to be addressed before we can merge this:

1. No tests.
The newly implemented True/False return behaviour of send_identify_remote_slave is not covered by any test. Please add tests following the pattern already established in test_lss.py.

2. Parameter naming.
nodelay=False is a double negative — delay=True (or similar) would read more naturally at call sites.

3. Delay removal may break slow devices.
The existing code comment explicitly states "some device needs these delays between messages because it can't handle messages arriving with no delay." Removing all inter-message delays may silently break compatibility with such devices. A short delay (e.g. 1–5 ms) between the first five messages would be a safer compromise while still keeping the scan reasonably fast.

4. Silent handling of unexpected responses.
When the response CS is neither CS_IDENTIFY_SLAVE nor causes a timeout, the function silently returns False. Other methods in this file raise LssError on an unexpected CS — please be consistent.

We would be happy to merge this once these points are addressed. If there is no update within the next few weeks, we will close the PR to keep the backlog manageable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants