The while (true) { await sleep(1); ...
is potentially very computationally expensive. Yes, it'd be good to refactor it out if possible.
Response bug Your current implementation looks like it has a bug, or what could very easily become a bug. The inbox
array is global, but each portInfo
array item will push separate, undistinguished items to the inbox
array. That is, each forEach
iteration will create a port
and open it immediately, and then whichever iteration whose sendAndReceive
happens to run its timeout first after the data
callback has pushed an item to the array will get the response. Here:
const response = await sendAndReceive(m, port);
Unless you have only a single portInfo
, the response
received is likely to correspond to a different iteration's m
and port
.
The easy way to fix this would be to declare a separate inbox
array inside each iteration - but the whole approach there deserves some reworking, as you noticed.
Inside the innermost loop that calls sendAndReceive
, you might construct Promises for each message being iterated over, and push their resolve function to an array. When the parser responds, if the array has a resolver function, shift it out and call it with the data:
portInfos.filter(pinfo => pinfo.manufacturer === 'FTDI')
.forEach(portInfo => {
const port = new SerialPort(portInfo.path).setEncoding('ascii');
const parser = port.pipe(new ReadLine({
delimiter: '\r\n',
encoding: 'ascii',
}));
const resolves = [];
parser.on('data', data => {
if (resolves.length) {
resolves.shift()(data);
}
});
port.open();
const serialMessage = api.createReadMessage(SERIAL);
const batteryMessage = api.createReadMessage(BATTERY);
for (const message of [serialMessage, batteryMessage]) {
new Promise((resolve) => {
port.write(message);
resolves.push(resolve)
})
.then((response) => {
console.log(m.trim());
console.log(response);
});
}
});
The promises are currently dangling, which is usually weird, but in this case, the promises will never reject. If parser
might not be able to get a data
for a given message, and it exposes an API for that (an error event, maybe?), it would be good to listen for that, so errors can be properly handled instead of ignored.
Note that the above approach calls port.write
twice at once in a given iteration *once for each method), instead of waiting for the prior message Promise to resolve first - this will speed up your program a bit, since you're now waiting in parallel, not in serial. If you wanted to do something when both messages are received, you'd use Promise.all
and pass into it an array of the Promises instead of using for..of
. (If you have to wait in serial, you can await
the resolution of each Promise inside the loop)
Other things I noticed:
for..await
is only for async iterators While you're technically allowed to do
for await (const m of [serialMessage, batteryMessage]) {
it doesn't make sense to use for await
, since the array there is just a plain array - it's not something that implements the async iterator interface. It'd be more appropriate to use just for (const m ...
Variable names m
isn't so great as a variable name. message
is significantly more informative.
Only use async
when you need to return a Promise You do:
SerialPort.list()
.then(async portInfos => {
portInfos.filter(pinfo => pinfo.manufacturer === 'FTDI')
.forEach(async portInfo => {
The .then
's callback does not use await
directly inside of it, nor does it need to return a Promise, so there's no need for the async
keyword - it's just potentially confusing noise.