Connect all Promise chains

[dominicbosch]

I really like your library, thank you for it!

However, the promise chains are broken in your code, which should be corrected. By chaining them all up, code depending on this module will be more reliable and also able to determine the status of actions. At the moment there is no way to determine whether initialization worked. By making a Promise out of the init method and adding it to the export object (API change... I know! but would be really added value!) a module depending on your code will already during initialization know whether everything is fine.

An API addition will also required, which outlines the new .init() method which returns a promise that allows to determine correct or false initialization.

[kaosat-dev]

Hi @dominicbosch ! Thanks for the kind words & the PR : I agree that promise chaining would be very useful ! Did you try changes in your PR with an actual device ? I remember trying to do more promise chaining but ran into some issues (silently failing to move a servo etc). I am ok with api /breaking changes btw, semantic version & releases are here for that :)

[dominicbosch]

Dear Mark,

I'm glad to hear that you are open to PR's and suggestions!

I am currently using your library in a project with my dad where we are running an autonomously driving RC car. He just quickly tested what I proposed and it seems to work very good. But since the changes are on a quite low level, we would have to do a more extensive testing once we sit together again within the next week.

The reason why I took such a close look at your library was because with the latest NodeJS version your code produces deprecation messages about unhandled Promises which will in the future cause the whole NodeJS procees to terminate!

(node:4033) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): TypeError: Failed to set address
(node:4033) DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
(node:4033) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): TypeError: Failed to set address
(node:4033) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 3): TypeError: Failed to set address
(node:4033) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 4): TypeError: Failed to set address

This means that failing promises are not having a .catch clause. The way I wire them together it is ensured that they all are connected and the user has to attach a catch clause. This allows him to deal with failures. So for every execute command (init, setPWM, setAllPWM, setPWMFreq, stop) the user receives a fully connected Promise that deals with all contained errors.

[dominicbosch]

sorry my laptop decided to click on close... I am reopening this because it was not meant to be closed...

[dominicbosch]

On another note:

These commands could actually be executed "concurrently" (I know there is no parallelism in NodeJS), and I guess on a hardware level it is "quite" sure that they are executed in the order they should.:

const setAllPWM = (on, off) => {
  i2c.writeBytes(ALL_LED_ON_L, on & 0xFF)
  i2c.writeBytes(ALL_LED_ON_H, on >> 8)
  i2c.writeBytes(ALL_LED_OFF_L, off & 0xFF)
  i2c.writeBytes(ALL_LED_OFF_H, off >> 8)
}

However, since you made the effort to wrap the i2c.readBytes and i2c.writeBytes into Promises (i2cWrapper.js), I suggest to exploit the full power of Promises and ensure they are executed one after the other in the order they are defined, already on the NodeJS application layer:

const setAllPWM = (on, off) => {
  return i2c.writeBytes(ALL_LED_ON_L, on & 0xFF)
    .then(() => i2c.writeBytes(ALL_LED_ON_H, on >> 8))
    .then(() => i2c.writeBytes(ALL_LED_OFF_L, off & 0xFF))
    .then(() => i2c.writeBytes(ALL_LED_OFF_H, off >> 8))
}

The callback within the .then clause ensures that the next i2c.writeBytes is for sure only executed after the last one succeeded. Beware of the pitfall that if the arrow functions are used without curly brackets () => i2c.writeBytes(*) (this works only for single commands, instead of the equivalent multi-line enabled () => { return i2c.writeBytes(*) }) that this implies a return of the result of the i2c.writeBytes(*) function. This return of Promises in the .then clause is crutial to keep the Promise chain connected. So basically above code is equivalent to this more verbose (showing more clearly what it is doing), longer but not necessarily more readable version:

const setAllPWM = (on, off) => {
  return i2c.writeBytes(ALL_LED_ON_L, on & 0xFF)
    .then(() => { return i2c.writeBytes(ALL_LED_ON_H, on >> 8) })
    .then(() => { return i2c.writeBytes(ALL_LED_OFF_L, off & 0xFF) })
    .then(() => { return i2c.writeBytes(ALL_LED_OFF_H, off >> 8) })
}

[kaosat-dev]

Hi @dominicbosch very busy week ,sorry I have not yet responded earlier! I will reply in depth to your posts later tonight !