Async DataRequest

[dumpfheimer]

Hey @puddly!

We were talking about async Data Requests over @ #174

I wanted to follow up on the idea and added a few lines (the else block) to the firmware in Application/mt/mt_af.c:

  if (MT_RPC_CMD_SREQ == (cmd0 & MT_RPC_CMD_TYPE_MASK))
  {
    MT_BuildAndSendZToolResponse(((uint8_t)MT_RPC_CMD_SRSP|(uint8_t)MT_RPC_SYS_AF), cmd1, 1, &retValue);
  } else {
    // It was an AREQ. If everything was successfull we should be fine.
    // If something failed we need to let the host know. We will do this by sending a DataConfirm message
    if (retValue != afStatus_SUCCESS) {
      //Build a DataConfirm message instead
      uint8_t retArray[3];

      retArray[0] = retValue;
      retArray[1] = dstAddr.endPoint;
      retArray[2] = transId;
      MT_BuildAndSendZToolResponse(((uint8_t)MT_RPC_CMD_AREQ | (uint8_t)MT_RPC_SYS_AF), MT_AF_DATA_CONFIRM, 3, retArray);
    }
  }

which in turn lets me add the following to zigpy-znp (in api.py in def request()):

        if type(request) is c.AF.DataRequest.Req and request.Callback is None:
            LOGGER.info("Sending DataRequest as AREQ %s " % type(request))
            request.header.with_type(t.CommandType.AREQ)

And it seems to work!
It now saves ~ 50 ms per request in the sync lock which can add up to a lot (especially when toggling a lot of lights or when polling fires many requests at once; often have this because I have about 15 smart plugs that measure electricity, 2 attributes per plug makes 30 simultaneous requests which would delay everything for abtou 1.5 seconds). With the "synchronous" request less than 1 ms is spent in the lock.

Would love to hear your thoughts

[dumpfheimer]

Not quite. Zigpy does not like the BUFFER_FULL callback. But I'm sure there would be a way around this:

zigpy_znp.exceptions.InvalidCommandResponse: Expected SRSP response AF.DataRequest.Rsp(Status=<Status.SUCCESS: 0>), got AF.DataRequest.Rsp(Status=<Status.BUFFER_FULL: 17>)

[puddly]

Interesting. My understanding of the Req and Rsp concept is that it provides software flow control. By removing the waiting for a Rsp, you're effectively just disabling the synchronous request lock for requests, which you can do right now without modifying the firmware one bit. I think everything still works when I last tried it, I'm just not sure if it's stable long-term.

I've just added two features to SerialPy that may be useful:

• a serialpy.patch() function that globally replaces the pyserial and pyserial-asyncio imports, allowing you to enable it once and affect all downstream modules.
• Enabling low-latency. I don't think it has any impact.

Can you add a start_time = time.monotonic() before a request is enqueued and sent, and log LOGGER.debug("Request took %0.4f", time.monotonic() - start_time) after? If you plot a histogram of the total request time with and without your individual changes, you should be able to see what real-world impact they actually have during runtime.

[dumpfheimer]

Good idea. I timed only the time in the sync lock and how long it takes to acquire the lock. Not the time it takes until a DataConfirm is received.

I did however look at the time it takes to toggle my living room lights which are 19 I believe and it looks like times for sending the requests went down from ~2s to ~1s. I also think that lights do not anymore turn on strictly in the same order but a tick more random.

[dumpfheimer]

I have been playing around quite a bit with a few different issues and managed to reduce my "test automation" time from 8s to ~2.5s. Just to be clear: the automation simply toggles a hand full of lights on and then off again (and waits for the responses).

I fiddled around with the send_packet function and ended up using source routing as primary attempt to reach the device. This seems to be extremely reliable and much faster. I also changed the DataRequestSrcRtg to be async in both, zipgy and the firmware.

Is the source routing approach discouraged for a specific reason?

[puddly]

and managed to reduce my "test automation" time from 8s to ~2.5s.

That's impressive!

I also changed the DataRequestSrcRtg to be async in both

I'm still not sure this is a safe thing to do. The request/response/callback flow provides higher-level software flow control to serial communication. If you remove the Rsp part, you're relying entirely on the request concurrency limits of the software communicating with Z-Stack to not overload it with serial data. I'd be curious to see how it behaves with multiple concurrent devices being OTA updated, and how timing is affected by removing the synchronous request lock from api.py but using the "normal" request command class.

Is the source routing approach discouraged for a specific reason?

Source routing bypasses firmware routing logic and just sends the packet over the provided path. I only added it in the most recent zigpy release and there is minimal logic at the moment to build source routes: https://github.com/zigpy/zigpy/blob/16ec072eb5e1e94dac33e5d408f939cf6746d652/zigpy/application.py#L599-L608

Once that's built up (and it will be tricky, since we have to consider degrading routes, routes immediately breaking due to offline devices, probabilistically inferring true link quality from application packet loss rates and scanned LQI, etc.), I think it can replace firmware routing with compatible coordinators.

[dumpfheimer]

There seems to be buffers for data requests. On the P7 I have a lot of memory and set them to rediculously high values but nevertheless there seems to be dedicated memory for the data. So far I have had no issues. But tbh the major part of the speed up seems to be the source routing. If the request does not lock up it takes 0.03s and I think if a route is provided there is little reason for the request to lock up. This then should in the worsed case add about 1s to the test automation. I understand that you are hesitant to trust it will work. A firmware change would come with its own issues anyway, although I will ask koenkk if he would add (accept) some kind of capability request and async data requests. They seem to work fine in a hybrid mode (I mean that the host can decide whether it's sync or async). Anyway the routing seems to have provided more stability (until now) over the async requests so that is definitely the safer bet.