-
Notifications
You must be signed in to change notification settings - Fork 582
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
Add HX711 I2C weight sensor module #1956
Conversation
Looks the same as: https://github.com/nanoframework/nanoFramework.IoT.Device/tree/develop/devices/Hx711 |
it's not HX711, it's I2C module on top of HX711 - HX711 is just backend |
src/devices/Hx711I2c/Hx711I2c.cs
Outdated
/// <param name="a0">Value of A0 pin.</param> | ||
/// <param name="a1">Value of A1 pin.</param> | ||
/// <returns>Address of the device.</returns> | ||
public static int GetI2cAddress(PinValue a0, PinValue a1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are 2 addresses, makes sens to have the second as a constant as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are 4, also they are physically labelled A0/A1 on this specific module
src/devices/Hx711I2c/Hx711I2c.cs
Outdated
/// DFRobot KIT0176: I2C 1kg Weight Sensor Kit | ||
/// </summary> | ||
[Interface("DFRobot KIT0176: I2C 1kg Weight Sensor Kit")] | ||
public class Hx711I2c : IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So really looks like the same as the one I4ve pointed out. Just your is SPI, the other one is I2C. So would be great to reuse the naming, align some, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's already mostly similar:
Offset - same
Tare - same
nano: Read; me: GetWeight
then there is couple of things they support and I don't and reverse
they have SampleAveraging I do support but do not expose directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also HX711 is GPIO and they seem to just use SPI for implementation. The module I have is only using HX711 as backend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added also SampleAveraging to match nano closer (also I found I needed that to get faster but less accurate sampling)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of comments but all up, looks good.
src/devices/Hx711I2c/Hx711I2c.cs
Outdated
/// <param name="a1">Value of A1 pin.</param> | ||
/// <returns>Address of the device.</returns> | ||
public static int GetI2cAddress(PinValue a0, PinValue a1) | ||
=> DefaultI2cAddress + ((bool)a0 ? 1 : 0) + ((bool)a1 ? 2 : 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should as well present the second I2C address as a constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think unless I add all 4 of them the second will be confusing while it may be obvious for me that second is A0=1, A1=0
some other person might think of second as A0=1, A1=1
or A0=0, A1=1
src/devices/Hx711I2c/Hx711I2c.cs
Outdated
[Property] | ||
public ushort AutomaticCalibrationWeight | ||
{ | ||
// It does not seem to be possible to read from this register |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even if not possible, you can store the data you write?
Same for the other ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't initially write anything for these two but I suppose I could initialize them in the ctor
src/devices/Hx711I2c/Hx711I2c.cs
Outdated
throw new InvalidOperationException("Getting weight failed. Getting bogus result."); | ||
} | ||
|
||
sum += (int)weightGrams; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a bit dangerous because of potential overflow. Don't you want to use the float and make the math average every time? few more operations but no risk of overload. A bit of loss in precision but if you're sampling many times, it's because the sensor is not very precise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed sum to 64-bit and it should not be possible anymore (SampleAveraging is 32-bit and sample is 24-bit which gives at most 56-bit so 64 should be enough) - having said that if someone uses uint.MaxValue as SampleAveraging this will take days to complete (I won't question what user wants though).
src/devices/Hx711I2c/Hx711I2c.cs
Outdated
/// </summary> | ||
/// <param name="samples">Number of times sampling occurs</param> | ||
/// <returns>Raw reading value</returns> | ||
public float GetRawReading(int samples = 20) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you have grams, then why not using UnitsNet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not grams, it's more like ADC reading (the docs don't say what this value is but ADC reading makes most sense). It will be gram-like after you divide that by CalibrationScale. Gram-like because unless you pick starting point (offset/tare/zero) the weight is kinda meaningless.
It's still useful to expose it so that Offset can be set/stored appropriately if needed. If I use grams here it will be very tempting to use this value where it's not supposed to be
using Iot.Device.Ft4222; | ||
using Iot.Device.Hx711; | ||
|
||
List<Ft4222Device> ft4222s = Ft4222Device.GetFt4222(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super minor thing, but should we have the sample not use Ft4222 since that isn't required and instead just have the sample read the sensor directly connected? Main reason why I mention this is for new people coming in that are not familiar with ft4222 and they may think that it is a prerequirement for getting this new sensor working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, we should eventually add link to article to every single I2C/SPI device how to use it. I can add a comment how you'd do that without FT4222 - generally I prefer to use samples I tested and I don't have any RPis handy at the moment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few minor comments, but looks good otherwise.
Check the Readme file for markdown errors. |
Note: this device is not actually HX711. It's an I2C module based on HX711. It comes by different names and doesn't have official chip name and is called with different names:
I used naming used by https://github.com/DFRobot/DFRobot_HX711_I2C
https://wiki.dfrobot.com/HX711_Weight_Sensor_Kit_SKU_KIT0176
The datasheet for this module is non-existing and code I was porting is not very well written and final result is a combination of C/Python and reverse engineering and I think it ended up much cleaner than original code
Microsoft Reviewers: Open in CodeFlow