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

Hypothetical patterns for asynchronous IO reads #28

Open
dtex opened this issue Jul 16, 2022 · 2 comments
Open

Hypothetical patterns for asynchronous IO reads #28

dtex opened this issue Jul 16, 2022 · 2 comments

Comments

@dtex
Copy link
Member

dtex commented Jul 16, 2022

This is a continuation of conversations had in:

On the call I offered to model some hypothetical API calls to a currently non-existent ECMA-419 asynchronous read API. To be clear, these are examples of I2C reads from the user's perspective. The inner workings of the read implementation are left to the implementor. We're using callbacks since not all target platforms will support promises, and error first call backs can easily be "promisified" in other host environments.

Existing ECMA-419 implementation

Synchronous

read(option[, stop])

Param. Required Range Default
option yes* positive integer, ArrayBuffer, TypedArray
stop no true or false true
  • The number of readable bytes is undefined so option is required
let result = myI2C.read(64);

This pattern makes perfect sense for embedded systems, or anywhere reads are fast enough not to be a problem while blocking.

Possible async patterns

Callback param

readAsync(option[, stop], cb)

Param. Required Range Default
option yes positive integer (number of bytes)
*sto no true or false.
cb yes err first callback
myI2C.readAsync(64, (err, data) => {
  // Do things with the data
});

This seems like the most practical option. By adding a distinct, optional method for asynchronous reads the absence of the method should throw in an unambiguous way. All of the other options listed below could result in implementations that do not throw an error but also do not behave as expected.


Variadic read

read(option[, stop][, cb])

Param. Required Range Default
option yes positive integer (number of bytes)
stop no true or false true
cb no err first callback* null
myI2C.read(64, (err, data) => {
  // Do things with the data
});

I believe that variadic functions make implementation, documentation, and support more difficult. I do not think we should use them here.


Leveraging onReadable

read(option[, stop])

Param. Required Range Default
option yes positive integer (number of bytes)
stop no true or false true

First read returns, when read is complete onReadable fires. This requires user code to manage state:

  • Does each read trigger a read or process the results of a read?
  • What register address was last read and which read request does this onReadable correspond to?
const myI2C = new device.I2C({
			...device.I2C.default,
			onReadable: () => {
     let data = myI2C.read(64);
     if data === null return;
     // do something with the data
			}
 });

let data = myI2C.read(64); // expets null

Because of the burden put on the user, I do not think this is a good pattern.

Adding an onread property

read(option[, stop])

Param. Required Range Default
option yes positive integer (number of bytes)
stop no true or false true

Read only triggers, when read is complete onReadable fires. This requires user code to manage state:

  • Does each read trigger a read or process the results of a read?
  • What register address was last read and which read request does this onReadable correspond to?
const myI2C = new device.I2C({
			...device.I2C.default,
			onReadable: () => {
     this.read(64);
   },
   onRead: (data) => {
     if data === null return;
     // do something with the data
			}
 });

let data = myI2C.read(64); // expets null

Because of the burden put on the user, I do not think this is a good pattern

@phoddie
Copy link
Collaborator

phoddie commented Jul 22, 2022

@dtex – thanks for putting together these excellent notes. It is helpful to be able to see different options side-by-side.

I completely agree with you that the onReadable and onRead approach is difficult to use. I implemented the onReadable approach for Fermata and, while it worked, there's no question that it was confusing to use.

That leaves the callback pattern and variadic read. They are mostly the same, if I understand correctly, apart from the method name.

On the call Wednesday, we talked a bit about read versus readAsync. The read in the I/O Class Pattern is already both synchronous and asynchronous, depending on the I/O. Because of that, while I agree with your point about the potential for confusion, I'm not sure we have another option.

I think the potential for confusion is reduced because on the call we also agreed that a given I²C instance must be either synchronous or asynchronous.

error first call backs can easily be "promisified" in other host environments

I don't have experience that gives me an intuition of how the order of the arguments in the callbacks simplifies "promisifying" of an API. It is a small point here, but I'd like to understand. Would you mind explaining?

As we talked about Wednesday, the design space here is bigger than read. In addition to asynchronous read, we need to consider asynchronous write. At a minimum, that would seem to require a callback to indicate success or failure of the call. More substantial, since SMBus is defined as an extension of I²C, we need consider how SMBus operates asynchronously as well.

I put together some notes to try to address the full space (thank you to @andycarle for reviewing and providing feedback). It is more-or-less an expansion of your variadic read. The constructor is explicitly passed a flag to enable asynchronous operation. That's necessary for compatibility, but it also has the benefit of making explicit that the instance is asynchronous (or synchronous) which may help a bit with the confusion concern.

@phoddie
Copy link
Collaborator

phoddie commented Aug 18, 2022

We had a very good discussion about this during the monthly TC53 call yesterday. I've updated the notes to reflect the outcome of that conversation. It feels like we're near a solution to integrate into the 2nd Edition draft and begin implementation work around.

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

No branches or pull requests

2 participants