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

feat: rewrite RyderSerial to work with the new bridge #15

Open
wants to merge 3 commits into
base: feat/ryder-dev
Choose a base branch
from

Conversation

pengowen123
Copy link

@pengowen123 pengowen123 commented Dec 29, 2022

RyderSerial now properly handles all the new bridge responses and is more organized and commented in general. The public interface has not changed significantly, so the web app component (in ryder-utils.ts) did not need many changes except to fix some small synchronization issues. However, the web app component will need to be improved in the future as well because it does not handle errors well.

Currently, COMMAND_SETUP cannot be used due to a protocol issue with the firmware, but all other commands should work. Also, signing in seems to succeed while still producing an error, but this appears to be an issue with the latest merged Hiro wallet changes and not with the new RyderSerial.

Fixes #14.

@pengowen123
Copy link
Author

Note that this requires a small change that will be pushed to the bridge after the rewrite PR is merged.

Copy link
Member

@MarvinJanssen MarvinJanssen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a first pass, let's see what @friedger says.

Comment on lines +212 to +213
// The `RyderSerial` must be closed before invoking the callback, or the latter might create a
// new `RyderSerial` and get a device busy error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When did this happen? Can we not check if it is already open and just keep it if so?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be possible and would be a better solution I think. However, I am unable to reproduce the original issue here. I remember inspecting the log and finding that instances were being created before the previous one was closed, leading to device busy errors, but it doesn't seem to be happening anymore.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the callback makes another call to Ryder, the previous RyderSerial might be still open. It is a good idea wrap the callback and help developers to do be able to call to Ryder from within the callback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah understood. Yes that would be a problem with the sequencer, it should deal with all those issues.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be easiest to make sure any user of RyderSerial only ever opens a single instance. Then, multiple commands can be sent in a sequence without issue:

let ryder = new RyderSerial(...);
let a = await ryder.send(...);
let b = await ryder.send(...);

This will require changes to ryder-utils, but I tried keep changes to it to a minimum in this PR.

on(event: string, callback: (data?: any) => void): this {
switch (event) {
case 'data':
this.socket.onmessage = messageEvent => {
console.log('new message from bridge', messageEvent.data);
messageEvent.data.arrayBuffer().then((d: Uint8Array) => callback(d));
callback(messageEvent.data);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it do the same thing as the old line?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The handling here was moved to happen later so that RyderSerial could distinguish between string and binary responses.

/* Handles responses received from the Ryder bridge and device. */
private on_response(response: any) {
if (typeof response === 'string') {
// String responses come from the bridge and are handled separately
this.on_bridge_response(response);
} else {
// Binary responses come from the device
response.arrayBuffer().then((data: Uint8Array) => {
this.on_raw_device_response.bind(this)(data);
});
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, although we should make all responses Uint8Array as on the other comment mentioned. Then the above is not necessary.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would require changes to the protocol, so this will have to stay until 0.0.6.

Comment on lines +33 to +37
export const BRIDGE_RESPONSE_WAIT_IN_QUEUE: string = 'RESPONSE_WAIT_IN_QUEUE';
export const BRIDGE_RESPONSE_BEING_SERVED: string = 'RESPONSE_BEING_SERVED';
export const BRIDGE_RESPONSE_SHUTDOWN: string = 'RESPONSE_BRIDGE_SHUTDOWN';
export const BRIDGE_RESPONSE_DEVICE_NOT_CONNECTED: string = 'RESPONSE_DEVICE_NOT_CONNECTED';
export const BRIDGE_RESPONSE_DEVICE_DISCONNECTED: string = 'RESPONSE_DEVICE_DISCONNECTED';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should define these in the protocol doc and turn them into error codes like above.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was initially planning on doing that, but I ran into an issue. The bridge responses may be received at any time, even while other data is being received. So if RyderSerial is in the middle of reading a multi-byte response, how should a bridge response byte be distinguished from normal data? It would be possible to adjust the protocol to require escaping these bytes, but I feel that the bridge should not influence the protocol because the two are independent. Also, as we are using WebSockets here, it is simple to just use different data types to distinguish them anyways (strings in this case).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The protocol should be adapted so that handling interrupts are specified. Interrupts will happen even more often with NFC

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above problem @pengowen123 mentions is currently not captured in the protocol spec. From memory, when this happened we used to just send a RESPONSE_OUTPUT_END, which would then result in an error state of incomplete data on the receiver and then handled (hopefully).

We need a RESPONSE_ERROR_DISCONNECT and variants so that apps can deal with it.

@friedger right, we will have to spec protocol version 0.0.6 to deal with those issues. I think we can start early considering the above.

Maybe we can group errors, and make errors responses two bytes instead of one. So for example, an error starting with 0xFE indicates a disconnect error and the next byte the kind.

src/app/ryderserial/ryder-serial/output_parser.ts Outdated Show resolved Hide resolved
Copy link

@friedger friedger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.
I'll do more review running the code..

Comment on lines +212 to +213
// The `RyderSerial` must be closed before invoking the callback, or the latter might create a
// new `RyderSerial` and get a device busy error
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the callback makes another call to Ryder, the previous RyderSerial might be still open. It is a good idea wrap the callback and help developers to do be able to call to Ryder from within the callback.

src/app/ryderserial/ryder-serial/index.ts Outdated Show resolved Hide resolved
src/app/ryderserial/ryder-serial/index.ts Outdated Show resolved Hide resolved
@MarvinJanssen
Copy link
Member

@friedger would it make sense to move this into a separate package again. We can just link it in package.json using a git link, we don't need to publish on npm yet.

Comment on lines +400 to +402
// Convert the result to a string
let result_string = Buffer.from(result).toString('binary');
resolve(result_string);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarvinJanssen This is now the only mention of strings when returning results to commands, so it will be easy to remove when the relevant changes are made to ryder-utils.

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