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

Current measurements via get_current are off by factor 4 under specific circumstances #10

Open
redfast00 opened this issue Apr 30, 2024 · 3 comments
Labels
bug Something isn't working documentation Improvements or additions to documentation

Comments

@redfast00
Copy link

If you first do set_shunt_calibration, then set_config(adc_range=ti.ina238.ADCRange.LOW), the SHUNT_CAL register is not updated. This causes the get_current function to return results that are off by a factor 4 (https://github.com/GSI-HPC/py-ina238/blob/main/src/ti/ina238.py#L278).

As a workaround, you can call set_config(adc_range=ti.ina238.ADCRange.LOW) first, then after that call set_shunt_calibration. Alternatively, you can use get_shunt_voltage and do the current calculation yourself in Python.

@dennisklein dennisklein added the documentation Improvements or additions to documentation label Apr 30, 2024
@dennisklein
Copy link
Member

Unfortunately, I have not yet found the time to add proper documentation nor unit testing (as well as support for the DIAG_ALRT register). But here is what the documentation should have provided: The existing API (so far) is designed as a very low-level and eager abstraction which merely adds some pythonic niceness on top of the raw device. So, no dependencies between registers such as you describe are really modeled yet. As a consequence the user has to be very familiar with the device and do things in the right order.

However, I was debating already with myself how a higher level API could/should look like but had no progress due to lack of time/priority. The eagerness of the current API also leaves out opportunities to skip any device communication which is effectively a no-op. On the other hand, it should give the most control which was the first concern for our internal use case.

I would like to leave this open to be sure to address this once documentation is produced - thank you!

@redfast00
Copy link
Author

I just encountered another issue:

self.inar = ti.ina238.Driver(I2CDevice(self.i2c, 0x43))
self.inal = ti.ina238.Driver(I2CDevice(self.i2c, 0x47))
self.inal.reset()
self.inar.reset()
self.inal.set_config(adc_range=ti.ina238.ADCRange.LOW)
self.inar.set_config(adc_range=ti.ina238.ADCRange.LOW)
self.inal.set_shunt_calibration(1.0, 0.04) # 1 ohm, max 40mA
self.inar.set_shunt_calibration(1.0, 0.04) # 1 ohm, max 40mA
self.inal.set_adc_config(avg_count=ti.ina238.Samples.AVG_64, vshunt=ti.ina238.ConversionTime.T_4120_US, mode=ti.ina238.Mode.CONTINUOUS_VBUS_VSHUNT_DIETEMP)
self.inar.set_adc_config(avg_count=ti.ina238.Samples.AVG_64, vshunt=ti.ina238.ConversionTime.T_4120_US, mode=ti.ina238.Mode.CONTINUOUS_VBUS_VSHUNT_DIETEMP)

This does not work as expected (this time, a factor of 4 difference between shunt voltage and current, but in the opposite direction as the previous issue), but if you add:

self.inal.get_config()
self.inar.get_config()

at the end, it does work. Getters should not impact internal state, but here they do (https://github.com/GSI-HPC/py-ina238/blob/main/src/ti/ina238.py#L210)

@dennisklein dennisklein added the bug Something isn't working label Apr 30, 2024
@dennisklein
Copy link
Member

I agree, would you like to provide a PR that moves this logic to the setter? Or, if you have something different in mind, let's discuss a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants