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

INA233 power monitoring driver #1250

Open
wants to merge 1 commit into
base: public
Choose a base branch
from
Open

Conversation

tve
Copy link
Contributor

@tve tve commented Nov 18, 2023

This PR adds support for the Texas Instruments INA233 power monitoring chip. It is typically used in battery systems to monitor battery voltage, current as well as overall power consumption. A special feature of this chip is to accumulate power internally allowing accurate
energy consumption measurements with relatively infrequent polling.

This driver is intended to conform with ECMA-419 and only supports a simple polling interface which is sufficient to keep track of what the battery pack is doing. It does not support a number of the chip's features, such as threshold alarms.

@phoddie
Copy link
Collaborator

phoddie commented Nov 21, 2023

This looks pretty great. It will be our first sensor implemented in TypeScript. ;)

I noted a couple of small things to correct. I'll write those up when I have a little time.

@phoddie
Copy link
Collaborator

phoddie commented Nov 21, 2023

  • onError. The IO callbacks (onReadable, onWritable, onError) are are not supposed to be called directly from an API. The idea is to avoid calling a function that then calls you back during its execution, as that is error prone (see "callbacks may not be invoked by the instance from within its public method calls, including the constructor" in the spec). Since this driver is fully synchronous, it doesn't need to use onError.
  • The call to readBuffer on line 105 will throw if the device isn't present. In that case, the #io will be orphaned.
  • The constructor is potentially modifying the input options. It should make a copy before doing that. (See "The constructor must not modify the options object. It must accept an immutable options object" in the spec)
  • But... the constructor is for setting up the hardware. Any changes to the default configuration should be applied by calling configure.

Combining these comments, you might rework the constructor:

  constructor(options: Options) {
	try {
		const io = (this.#io = new options.sensor.io({
		  hz: 400_000,
		  address: 0x40,
		  ...options.sensor,
		}))

		// verify that we're talking to an ina233
		const modelBuf = this.#io.readBuffer(COMMANDS.MFR_MODEL, 7)
		const model = new TextDecoder().decode(new Uint8Array(modelBuf, 1))
		if (model !== "INA233")
			throw new Error(`Wrong model: ${model}`)

		this.#io.sendByte(COMMANDS.RESTORE_DEFAULT_ALL)

		// ensure certain configuration options get set now
		this.configure({shuntOhms: 0.01, maxCurrent: 10, averaging: 1});
	}
	catch (e) {
		this.close();
		throw e;
	}
  }
  • get sampleInterval: This is defined to be in microseconds. ECMA-419 uses milliseconds for time everywhere it possibly can. This is the default in JavaScript. Being consistent in the APIs avoids confusion about units.

@tve
Copy link
Contributor Author

tve commented Nov 22, 2023

Thanks for the review! I think I need an "ECMA-419 driver checklist" 😆

@phoddie
Copy link
Collaborator

phoddie commented Nov 22, 2023

Thanks for the review! I think I need an "ECMA-419 driver checklist" 😆

Agenda item for the December 6 workshop: "Making ECMA-419 more approachable for developers". This is exactly the kind of thing that we should do. (cc @HipsterBrown @dtex).

@tve
Copy link
Contributor Author

tve commented Nov 28, 2023

Fixing this driver may take a little while 'cause of a testing bottleneck on my end. I only have one INA233 chip operational (dead-bugged) and I'm not sure I want to mess-up the firmware of the device it's in... I should make a breakout board...

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.

None yet

2 participants