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.

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

[BUG] linux Chip-tool app should not by default register ICD #31992

Closed
doublemis1 opened this issue Feb 7, 2024 · 14 comments
Closed

[BUG] linux Chip-tool app should not by default register ICD #31992

doublemis1 opened this issue Feb 7, 2024 · 14 comments
Labels
bug Something isn't working chip-tool icd Intermittently Connected Devices

Comments

@doublemis1
Copy link
Contributor

doublemis1 commented Feb 7, 2024

Reproduction steps

  1. Commission LIT device using non-interactive chip-tool
  2. Try to read vendor-id <Sigma 1 re-transmission every 200ms> -> timeout

Bug prevalence

always

GitHub hash of the SDK that was being used

0ff709f

Platform

raspi

Platform Version(s)

No response

Anything else?

Currently the chip-tool app register itself (commissioner) on the DUT in the ICD Management ActiveClient during commissioning.
The process of adding a active client in ICD Management is not a part of the commissioning itself (based on specification).
Current controller is able to skip registration by additional command parameter (--skip-icd-registration).
The controller should register icd if addtional flag is provided or by other chip-tool command

@doublemis1 doublemis1 added bug Something isn't working needs triage labels Feb 7, 2024
@mkardous-silabs
Copy link
Contributor

@doublemis1 The ICD registration process is defined in the ICD Client behavior section that has not been published yet.
The design decision for the chip-tool was to regiter by default when the comissionee is a LIT ICD since that is the behavior client should have.

Do you think this is still bug with the additional information?

@mkardous-silabs mkardous-silabs added chip-tool icd Intermittently Connected Devices and removed needs triage labels Feb 7, 2024
@doublemis1
Copy link
Contributor Author

I think that currently with the ICD using non-interactive chip-tool is no-go for LIT.
if it is expected then implementation of chip-tool need to be updated to store in controller settings the LIT's idle duration (if the controller has been registered) to not based the re-transmission time on default behavior (not taking into account LIT's idle duration)

@doublemis1
Copy link
Contributor Author

Can you share also the PR to the specification to familiarize with new Commissioning procedure

@bzbarsky-apple
Copy link
Contributor

currently with the ICD using non-interactive chip-tool is no-go for LIT.

You mean without passing --skip-icd-registration (because there's no queuing of interactions implemented)? Or in general? If the latter, why?

That said, given how chip-tool is normally used, and given the fact it has no queuing implemented and no way for clients of the non-interactive chip-tool to implement it because they have no way to get hold of the relevant data, it might in fact make sense to have chip-tool default to not registering @mkardous-silabs

@doublemis1
Copy link
Contributor Author

doublemis1 commented Feb 7, 2024

currently with the ICD using non-interactive chip-tool is no-go for LIT.

You mean without passing --skip-icd-registration (because there's no queuing of interactions implemented)? Or in general? If the latter, why?

That said, given how chip-tool is normally used, and given the fact it has no queuing implemented and no way for clients of the non-interactive chip-tool to implement it because they have no way to get hold of the relevant data, it might in fact make sense to have chip-tool default to not registering @mkardous-silabs

Right, without --skip-icd-registration is a no-go.
Of course if the chip-tool store the data about LIT's idle mode duration and based on that, defines the re-transmission intervals then it should works too.

@mkardous-silabs
Copy link
Contributor

mkardous-silabs commented Feb 7, 2024

@bzbarsky-apple @doublemis1 Queuing is beeing added in this PR #31898
We still have a few pieces missing to fully support LIT ICDs in the SDK.

I'm not sure i agree we should change the default behavior since the goal was in fact to register by default...
Clients that can't support LIT ICD, imo, should be the one to implement a "different" behavior.

For the test harness / certification, we are planning on using the test event trigger to force the ICD in different states (active mode) to be able run cert tests that don't validate ICD behavior. It's on the Todo list for 1.3.

In my opignion, this is isn't a bug. It is work in progress but fundamentally it is doing what was we intended.

@yunhanw-google @erjiaqing you might be interested by this conversation.

@doublemis1
Copy link
Contributor Author

But currently the case with non-interactive chip-tool will work as follow:

  1. Commissioning the device using chip-tool (including register of controller as a client)
  2. Send any zcl command/read/write, the timeout on controller side due to immediate retrnasmissions -> finally on device wake up, the stored packets (on e.g. border router) starts the CASE Session -> the CASE Session invalid after x seconds (within that time new CASE session cannot be established)

@mkardous-silabs
Copy link
Contributor

Yes, that is true.
We still need to document how to interact with lit-icds but that is the comprise we accepted to have the preferred client behavior on the chip-tool.

@doublemis1
Copy link
Contributor Author

@mkardous-silabs
ok, I just wanna point out the possible issues with the usage of the tool.
And additionally I looked at the specification regarding ICD Behavior and Commissioning process.
I have one question, is it be suitable to mention in commissioning flow section about potential possibility of registering ICD device.
Correctly the process seems be divided in to separate sections, where if you don't tell me that the ICD device can be registered during commissioning, I will missed that if I want to know how commissioning procedure should be implemented.

@doublemis1
Copy link
Contributor Author

@mkardous-silabs @bzbarsky-apple
Is there any option to decide if there something to be done, to have better UX with chip-tool and LIT device (it will be the best to have it for Matter 1.3 commit sha)?

  1. Chip-tool saves in tmp/configuration files about registered LIT device and its Idle mode duration, and use that for retransmission intervals
  2. by default skip icd registration
  3. add comprehensive documentation about potential issues with chip-tool and LIT

@yunhanw-google
Copy link
Contributor

yunhanw-google commented Feb 16, 2024

Just read through this bug, I would prefer going with ICD configuration at default if device is LIT, after #31898 is landed(still under development), we are able to automatically queue IM operations and flush whenever receiving the corresponding check-in message. For queue, we plan to add active mode duration to let user know how long they can operate in the active window, then re-queue again. In other words, we would automatic the flow at default without additional setup/parameters. Btw, we are doing the same thing using android-chip-tool with UI there.

When using skip-icd-registration, we would support to do ICD registration manually via this ticket, #32187, we would queue manually.

Documents would be added in tool itself for better track.

@mkardous-silabs
Copy link
Contributor

mkardous-silabs commented Apr 5, 2024

After discussing this with @tennessee-google @bzbarsky-apple and @leorozendaal, this is the new proposal for the Chip-tool behavior.

  1. Default behavior is not to register with the ICD that supports the ICD Check-In protocol. Chip-Tool flag can be passed to register at commissioning
  2. Add costum icd register and icd unregister command to enable interactions with the chip-tool storage. This would also allow to register different information than the chip-tools - [ICD] If chip-tool sends the register-client command, the information is not stored in the ICD storage #31697
  3. Add custom icd start-stay-active and icd stop-stay-active commands. These are used to enable the chip-tool to send periodic stay-active-request to keep the ICD in active mode for the user to send the commands he needs.
  4. Add action when Chip-tool receives a Check-In message to send a stay-active-request command immediatly after.
  5. Add action when Chip-tool receives a sub report to send a stay-active-request command immediatly after.

3, 4 and 5 would be restricted to the chip-tool being in interactive mode.

When this design, we won't any queue logic. Closing #30936 as not doing.

fyi @yunhanw-google @ericzijian1994

@yunhanw-google
Copy link
Contributor

WG discussion: update icd start-stay-active with duration parameter, and remove icd stop-stay-active, these are used to enable the chip-tool to send periodic stay-active-request to keep the ICD in active mode for the user to send the commands he needs until the duration is reached.

@mkardous-silabs
Copy link
Contributor

Closing this issue since the fixes were merged into master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working chip-tool icd Intermittently Connected Devices
Projects
None yet
Development

No branches or pull requests

4 participants