Add "write" operation to HAConnection

[bgoncal]

No description provided.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.87%. Comparing base (48e36a0) to head (476a426).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #55   +/-   ##
=======================================
  Coverage   99.87%   99.87%           
=======================================
  Files          50       51    +1     
  Lines        1593     1628   +35     
=======================================
+ Hits         1591     1626   +35     
  Misses          2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[zacwest]

This sort of breaks expectations. Is there something the library needs to do that isn't handled right now? Exposing the raw WebSocket for writing data doesn't seem like a good idea to me.

[bgoncal]

This sort of breaks expectations. Is there something the library needs to do that isn't handled right now? Exposing the raw WebSocket for writing data doesn't seem like a good idea to me.

To integrate with Assist pipeline I need to be able to write binary data to the websocket

[zacwest]

Can you add a request type that can do that?

[bgoncal]

Can you add a request type that can do that?

No problem on my side, but what would be the downside of keeping the way it is?

[zacwest]

I think the biggest issue is it gives unfettered access to the underneath, and requires consumers to format appropriately. It's like an escape hatch but a dangerous one. I think it also means the callbacks that may trigger are unexpected or confusing to the library, if any happen.

[bgoncal]

But consumer will still not be able to do anything else than writting data, if I make a request type for that, the access level will be the same since this request wont have any other information than the binary data itself.

[zacwest]

Does the data get decorated in any way to indicate what kind of payload it is or what it's used for?

[bgoncal]

The only modification to the data is a prefix of 1 byte which indicates the assist handler id, literally e.g. "01"

[bgoncal]

@zacwest check my last commit, I made a proposal, even though I don't think this abstraction is really needed, let me know what you think

[zacwest]

Can you share with me an example data write so I can maybe understand? The write needs to contain something like "use this data for purpose X" right?

[bgoncal]

Can you share with me an example data write so I can maybe understand? The write needs to contain something like "use this data for purpose X" right?

https://developers.home-assistant.io/docs/voice/pipelines/#sending-speech-data

[zacwest]

Is there a mechanism to get responses to that binary data?

My recommendation would be: codify that 'handler id' (the 01 in that example) as a separate field in the 'write data' parameter, so you'd do a 'request with data' + + <here's the data>

[bgoncal]

Is there a mechanism to get responses to that binary data?

My recommendation would be: codify that 'handler id' (the 01 in that example) as a separate field in the 'write data' parameter, so you'd do a 'request with data' + + <here's the data>

Just a quick overview of how the flow currently works:

You subscribe to something like:

private func assistByVoiceTypedSubscription() -> HATypedSubscription<AssistResponse> {
        .init(request: .init(type: .webSocket("assist_pipeline/run"), data: [
            "pipeline": preferredPipelineId,
            "start_stage": "stt",
            "end_stage": "tts",
            "input": [
                "sample_rate": audioSampleRate,
            ],
        ]))
    }

Then it will have many events back such as "run-start, stt-start ... and others", when you receive the stt-start, it means you can start writting binary data and you dont get anything back from it, the way the user get's feedback is through events that will be triggered later on such as "stt-end, run-end.. and others". So thats why I don't think we need anything more complex than just writting data directly, if use cases grow then this can be improved. Let me know what do you think

[zacwest]

That makes sense. I think of this library as doing whatever is necessary to make HA-specific things happen in the connection. So in this case, I think it is actually quite valuable to say "send a stt request with voice data" without the caller needing to know how to do that. It's not a super heavy wrapper (just adding that handler id prefix and writing data) but it abstracts away a part which could, in theory, change or be made more complex later.

[bgoncal]

That makes sense. I think of this library as doing whatever is necessary to make HA-specific things happen in the connection. So in this case, I think it is actually quite valuable to say "send a stt request with voice data" without the caller needing to know how to do that. It's not a super heavy wrapper (just adding that handler id prefix and writing data) but it abstracts away a part which could, in theory, change or be made more complex later.

Ok I replaced my implementation with some more specific code for sending stt audio data, let me know what you think

[zacwest]

0b100 - 0b10 is already taken above

[bgoncal]

0b11 then right? Why aren't we just using integers here?

[home-assistant]

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

[bgoncal]

@zacwest can you check this one again?

[zacwest]

I mean this literal closure here. The completion block for send() - can you test it is invoked?

[bgoncal]

The original implementation of the "send" method didn't have any usage of the completion. Should this be altered?

[zacwest]

I'm guessing this isn't invoking the completion blocks, but it may be useful to either wait to invoke it until the write occurs, or just invoke the completion right away. I know the server isn't ACK-ing this but it's still good form to be able to chain these together for throttling if you want.

[bgoncal]

Communicating to the response controller isn't enough? like done here:

 private func sendWrite(_ sttBinaryHandlerId: UInt8, audioDataString: String?) {
        // If there is no audioData, handlerID will be the payload alone indicating end of audio
        var audioData = Data(base64Encoded: audioDataString ?? "") ?? Data()

        // Prefix audioData with handler ID so the API can map the binary data
        audioData.insert(sttBinaryHandlerId, at: 0)
        workQueue.async { [connection] in
            connection?.write(data: audioData) { [weak self] in
                self?.responseController.didWrite()
            }
        }
    }

I noticed that when "sending" using websocket or rest, that's the only response, completion is not used for those either

[zacwest]

Not all of the tests check the invocation of the completion handler, but some do. e.g. testPlainSendSentSuccessful, testPlainSendSentFailurePromise, etc. You're making a new one with a unique code path, so you need to make sure all of the same guarantees happen somewhere.

The response controller only invokes based on identifiers - there's no identifier/callback for this write, so there's no way for it to know what to call back through.

[bgoncal]

Ah, got it, ok I added the identifier and a test to validate it, I also added this code block inside "sendWrite":

 if let identifier, let request = self.requestController.single(for: identifier) {
                    callbackQueue.async {
                        request.resolve(.success(.empty))
                    }
                    requestController.clear(invocation: request)
                }

Because I can't add identifier to the data dictionary (since I am sending binary in this write) and it will also have no websocket event back from the connection.

Does that make sense?

[zacwest]

Lgtm now!