Implement v1.0.0 spec #13
Conversation
| } | ||
| } | ||
|
|
||
| if (handler == null) { |
There was a problem hiding this comment.
per the other discussion, i do not believe this is an if/else situation.
Separately, I don't understand Kotlin. How does the registered functionCallHandler pass back the resultData used in the resultHandler?
Also note that in client-js/ios, I don't believe we are providing built-in support for sending back a result for the generic onLLMFunctionCall callback.
There was a problem hiding this comment.
Yeah, I'll update this.
How does the registered functionCallHandler pass back the resultData used in the resultHandler?
We give the function call handler two arguments, the functionCallData, and onResult, the latter of which is a function (defined as the lambda resultHandler above). The function call handler invokes onResult(value) with the value once it's done.
Also note that in client-js/ios, I don't believe we are providing built-in support for sending back a result for the generic onLLMFunctionCall callback.
This would make sense (to avoid two responses), however I think currently the iOS client does currently accept a result in the callback (tagging @filipi87):
func onLLMFunctionCall(functionCallData: LLMFunctionCallData, onResult: ((Value) async -> Void)) async
| else -> { | ||
| Log.w(TAG, "Unexpected message type '${msg.type}'") | ||
|
|
||
| callbacks.onGenericMessage(msg) |
There was a problem hiding this comment.
We did away with the onGenericMessage handler in 1.0.
There was a problem hiding this comment.
👍 Will remove.
| TransportState.Connected, | ||
| TransportState.Ready -> return@runOnThreadReturningFuture resolvedPromiseErr( | ||
| thread, | ||
| PipecatError.InvalidState( |
There was a problem hiding this comment.
you might be right that a better name for this type of error is a PipecatError, but if we want to be consistent, they are RTVIErrors in the client-js API.
I'm starting to feel like a lesson-learned from this is for all platforms to update simultaneously so we work out these oddities and overlooked corners before we're stuck with them simply because "it's the way i did it 🤷♀️ "
There was a problem hiding this comment.
In my defense, I think it's because we have one Error type and I think my hands were tied because i needed to define the Error type as part of the RTVI portion of the library due to dependencies in the RTVIEvents
There was a problem hiding this comment.
😂 Okay, I'll rename this back to RTVIError for consistency.
| thread = thread, | ||
| url = startBotParams.endpoint, | ||
| body = JSON_INSTANCE.encodeToString(startBotParams.requestData) | ||
| .toRequestBody("application/json".toMediaType()), |
There was a problem hiding this comment.
is "application/json" not part of the headers?
There was a problem hiding this comment.
Yes, this is just a quirk of the OkHTTP API and the headers get set as a result of this.
| /** | ||
| * Initiate an RTVI session, connecting to the backend. | ||
| */ | ||
| fun connect(transportParams: Value): Future<Unit, PipecatError> = |
There was a problem hiding this comment.
nit: transportConnectionParams or connectParams
There was a problem hiding this comment.
Ah -- I copied this from the iOS API which uses transportParams. Happy to change if we can settle on a name, maybe connectParams? Because Swift and Kotlin use named params this is an API break if we update iOS retroactively. Tagging @filipi87.
There was a problem hiding this comment.
not worth breaking on a nit. the js signature is:
public async connect(connectParams?: TransportConnectionParams): Promise<BotReadyData>
which is where i conjured up those names, but probably better to prioritize android being consistent with ios vs. js
There was a problem hiding this comment.
Okay, will leave as transportParams in that case.
| * Initiate an RTVI session, connecting to the backend. | ||
| */ | ||
| fun connect(transportParams: Value): Future<Unit, PipecatError> = | ||
| thread.runOnThreadReturningFuture { |
There was a problem hiding this comment.
this should also throw an error if the state is already authorizing/authorized/connecting/connected
There was a problem hiding this comment.
nm. maybe the logic below actually does that.
There was a problem hiding this comment.
Yeah, I think it's currently relying on the existence of the connection object which seems cleaner imo than checking if we're in one of the 4 of 9 acceptable states.
| ) | ||
| } | ||
|
|
||
| Log.w(TAG, "No connect endpoint specified, skipping auth request") |
There was a problem hiding this comment.
Oops, well spotted.
|
|
||
| Log.w(TAG, "No connect endpoint specified, skipping auth request") | ||
| connection = Connection() | ||
| return@runOnThreadReturningFuture transport.connect(transportParams) |
There was a problem hiding this comment.
where are devices initialized on connect()?
There was a problem hiding this comment.
We do this inside the transport (to avoid the outer client needing to keep track of whether it was already done)
| val arguments: List<Option> | ||
| data class ClientAbout( | ||
| val library: String, | ||
| @SerialName("library_version") |
There was a problem hiding this comment.
just curious, what does this resolve to?
There was a problem hiding this comment.
I'm setting this in the transports (not pushed yet), to the version of the transport library ("1.0.0" in this case).
| ) | ||
|
|
||
| @Serializable | ||
| data class BotLLMSearchResponseData( |
There was a problem hiding this comment.
our type here is very different. i'm trying to understand if they should be? something tells me yours is more accurate, but then again, i know i've tested this and it worked on client-js 🤔
There was a problem hiding this comment.
Hmm, that's weird. I based these on the types in the iOS client here:
Which differ dramatically from the web version:
Since you've tested these I'm happy to change the Android ones to match. It would be good to understand how the difference arose though (maybe the iOS ones are left over historically?)
There was a problem hiding this comment.
but i also see these fields in the pipecat GoogleLLMService code. so don't delete yet. i'll see if i can dig.
No description provided.