Add big-endian and little-endian variants of readWord and writeWord

[fivdi]

Add the following methods to class Bus

• readWordBE
• readWordLE
• readWordSyncBE
• readWordSyncLE
• writeWordBE
• writeWordLE
• writeWordSyncBE
• writeWordSyncLE

Add the following methods to class PromisifiedBus

• readWordBE
• readWordLE
• writeWordBE
• writeWordLE

[johntalton]

Given that an implementation exists on Buffer ... is this appropriate.

in fact, preferences would be to see the API reduce the nodejs deps and return ArrayBuffer or one of the TypedArray's. but alas, i do not have that PR ready :)

[fivdi]

Given that an implementation exists on Buffer ... is this appropriate.

Yes, I think it is appropriate. Class Bus already has the methods readWord, readWordSync, writeWord and writeWordSync. In addition, class PromisifiedBus already has methods readWord and writeWord. The current implementations of these methods are either all big-endian or all little-endian, I can't actually recall which. Curently, none of these methods rely on Buffer. The readWord* methods return a number and the writeWord* methods are passed a number. This is a simple interface and it should stay that way. Modifying the interfaces of these methods to use Buffers rather than numbers would make things more complex and more difficult to use.

in fact, preferences would be to see the API reduce the nodejs deps and return ArrayBuffer or one of the TypedArray's. but alas, i do not have that PR ready :)

As mentioned, the APIs which are the subject of this issues rely on numbers rather than Buffers which makes the interface simpler and easier to use so it should stay that way. There are other methods that rely on Buffers but that would be a different issue. What I don't want to do is make breaking changes to any of the APIs.

[johntalton]

indeed my own bias shows through. I avoid r/w Word calls. And focus on Byte or Buffer api via a helper class I2CAddressedBus

Which maps to (i2cRead, i2cWrite, sendByte, readI2cBlock and writeI2cBlock)
i also use some non-addressed method (like deviceId 😄)

these five methods seem to cover most use cases (i have over a dozen I²C devices which i have written drivers for attached to a Pi4 and they all can be accessed via these methods only)

I share the above only to provide an example Use case. Im sure many other use the api differently.

Now, as for readWord (and friends).

The i2c_smbus_access layer and above performs NO conversions (aka i2c_smbus_data IS buffer .. or at least a _u8 array of I2C_SMBUS_BLOCK_MAX [actually 34]). Well, minus the Nan cast into a _s32.

This can be considered vendor-format as every device (and sometimes on the same device) defines the byte ordering.

ARCH	Device	readWord	readWordLE	readWordBE
LE	LE	LE	LE	swap to BE
LE	BE	BE	BE	swap to BE
BE	LE	LE	swap to BE	LE
BE	BE	BE	swap to BE	BE

With strikeouts being logic errors on the application side - maybe. This seems to work ok.

This has the code-smell of application logic (or at least the first layer firmware) and prefer to keep all data in vendor-format until the final parsing (this stems from the experience that the majority of devices rarely have a "clean" word register, most are encoded / shifted and/or masked in some way requiring bit access to the "buffer").

Lastly, much of this is to play devils-advocate here for the ArrayBuffer world-view. Compatibility, Stability and Utility are far more powerful motivating factors. Though maybe the data-manipulation layer can be extracted to an options wrapper class (aka I2cBusRaw and I2cBus, though that sounds like a significant PR also and would likely be better as a fork).

As alway, thanks 🥳.
Maybe you should even consider adding some yml

[johntalton]

Alternatively to reduce namespace pollution etc:

const i2c1 = await i2c.openPromisified(1)
const word = await i2c1.withByteOrder(BIG_ENDIAN).readWord(addr, cmd)

const i2c1 = i2c.open(1, err => 
  if(err) ...
  i2c1.withByteOrder(LITTLE_ENDIAN).readWord(addr, cmd, word => {
    // 
})

const i2c1 = i2c.openSync(1)
const word = i2c1.withByteOrder(HOST_ENDIAN).readWord(addr, cmd)

• Additional updates to open to return proper wrapper class should be trivial (following the openPromisified pattern?)

A refactor of Sync methods under its own new wrapper class could be done, though aliases for api stability should be added (or the CB version extends the Sync version to replace inline). (SyncBus, CBBus, PromisifiedBus ?)

• Default class should match HOST_ENDIAN (as the api does this today [verify?]).

Setting the default could take the following form:

const i2c1_BE = i2c.openPromisified(1).withDefaultByteOrder(BIG_ENDIAN)

• A refactor attempt could include breaking monolithic javascript into class files (design preferences?)

🤷

[fivdi]

@johntalton Thank you for the feedback.

I have looked at this a little closer and now realize that I went off on a bit of a wild goose chase here. The issue isn't really a big-endian / little-endian issue.

Section 6.5.4 of the SMBus Specification states that the write word protocol is:

S Addr Wr [A] Comm [A] DataLow [A] DataHigh [A] P

Similarly, section 6.5.5 states that the read word protocol is:

S Addr Wr [A] Comm [A] S Addr Rd [A] [DataLow] A [DataHigh] NA P

The exact position of the low byte (Bits [7:0]) and high byte (Bits [15:8]) is clearly defined. If a device adheres to the protocol and the operating system / kernel driver do their job correctly, everything will function as expected irrespective of whether the host architecture is big-endian or little-endian.

Of course, there will be devices that do not adhere to the protocol resulting in the bytes being the wrong way around. In such cases, the bytes will be the wrong way around irrespective of whether the host architecture is big-endian or little-endian.

This is further confirmed by the Linux SMBus Protocol Summary which has the following to say about i2c_smbus_read_word_data:

SMBus Read Word:  i2c_smbus_read_word_data()
============================================

This operation is very like Read Byte; again, data is read from a
device, from a designated register that is specified through the Comm
byte. But this time, the data is a complete word (16 bits).

S Addr Wr [A] Comm [A] S Addr Rd [A] [DataLow] A [DataHigh] NA P

Functionality flag: I2C_FUNC_SMBUS_READ_WORD_DATA

Note the convenience function i2c_smbus_read_word_swapped is
available for reads where the two data bytes are the other way
around (not SMBus compliant, but very popular.)

Adding four new methods to Bus and two new methods to PromisifiedBus would resolve the issue.

Add the following methods to class Bus

• readWordSwapped
• readWordSwappedSync
• writeWordSwapped
• writeWordSwappedSync

Add the following methods to class PromisifiedBus

• readWordSwapped
• writeWordSwapped

[johntalton]

I also was working under the assumption that the device native order was variable. SMBus indeed has an opinion here which to lean on.

Would it be useful for the consumer of the API to have a more descriptive interface (similar to my original proposal preserving the BE/LE suffix), that allows the callers code to indicate the byte order, but also set default (preserving all existing apis etc).
If it was confusing to us, it may be to others?

Otherwise... if you head down the path of wrapping existing methods to create the new api:

readWordSwapped(addr, cmd, cb) {
  this.readWord(addr, cmd, (err, rawData) => {
    if(err) { return cb(err) }
    return cb(null, swapWord(rawData))
  })
}

readWordSwappedSync(addr, cmd) {
  return swapWord(this.readWordSync(add, cmd))
}

writeWordSwapped(addr, cmd, word, cb) {
  this.writeWord(addr, cmd, swapWord(word), cb)
}

writeWordSwappedSync(addr, cmd, word) {
  return this.writeWordSync(addr, cmd, swapWord(word))
}

promise version

readWordSwapped(addr, cmd) {
  return this.readWord(addr, cmd).then(swapWord)
}

writeWordSwapped(addr, cmd, word) {
  return this.writeWord(swapWord(word))
}

quick and dirty

function swapWord(word) {
  checkWord(word)
  if(word == 0) { return 0 } // short circuit not needed 
  return ((word & 0x00FF) << 8) | ((word & 0xFF00) >> 8)
}

test-ish

describe('swapWord', () => {
  it('should throw on invalid Word', () => {
    expect(() => swapWord('not an integer')).to.throw()
    expect(() => swapWord(3.14)).to.throw()
    expect(() => swapWord(null)).to.throw()
    expect(() => swapWord(0xFFFF + 1))
  })

  it('should preserve Zero', () => {
    expect(swapWord(0)).to.equal(0)
  })

  it('should swap all know values', () => {
    // flatten(range(0, 0xFF).map(i => range(0, 0xFF).map(j => ({ exp: ..., swap: ... }))))
    for(i = 0; i < 0xFF; i++) {
      for(j = 0; j < 0xFF; j++) {
        const swappedWord = j << 8 | i
        const expectedWord = i << 8 | j 
        expect(swapWord(swappedWord).to.equal(expectedWord))
      }
    }
  })
})

_{* please excuse any mistakes, written in browser :)}

Given the amount of added code this way for, basically the addition of the swapWord method, it seems heavy. An argument could be made that this is Application logic at this point and should be pushed into that layer .. keeping this library "clean" (back to my original point of not adding anything :P)

If this is half-close to your code let me know and we can create a PR.

[lynniemagoo]

I offered a thumbs up above regarding the simpler API. I have also wondered about this. If you really needed to do something here, I'd recommend using bitwise operations to do the swap.

Assuming you know how to break apart 16 bits, into variables x and y.

(((x) ^= (y)), ((y) ^= (x)), ((x) ^= (y))); // No-temp-var swap operation

In NodeJS, depending on the OS, you have platform-specific Endian as well as platform specific-EOL.

There are indeed many things at play here such as byte ordering on the I2C Bus as well as bit ordering within a byte and, IMO, adding layer upon layer of BE vs LE only adds confusion unless of course you are checking the platform endianness.

There's nothing that says you could not wrap the methods you want with a mixing and weave the behavior you desire. I have to agree with keeping things simple with regard to 'number' data type.