Closed slabula closed 5 years ago
Whoooahh!!! This is quite a bit pull request. Looks like you've committed multiple modifications of different nature all into one pull request. This is a bit odd and quite a challenge to review. For what I can see, this looks quite like a whole refactoring of this little file.
Any chance you could share a brief and clear description of the features submitted here?
Thanks!
I'm new to git and had made a bunch of changes before figuring out how to commit back.
I'll write up the changes tonight.
Cheers, Gordon
On Tue, 12 Feb 2019, 1:22 pm tatobari <notifications@github.com wrote:
Whoooahh!!! This is quite a bit pull request. Looks like you've committed multiple modifications of different nature all into one pull request. This is a bit odd and quite a challenge to review. For what I can see, this looks quite like a whole refactoring of this little file.
Any chance you could share a brief and clear description of the features submitted here?
Thanks!
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tatobari/hx711py/pull/19#issuecomment-462756690, or mute the thread https://github.com/notifications/unsubscribe-auth/ApBt4YlyHFvsYXYXBqyuQRg15_eYRfgZks5vMsASgaJpZM4ay2dg .
Great! For example, I've been wanting to remove numpy. Shouldn't be much of a problem. It's just a matter of shifting bits. However, all the other functionalities are mainly out of my radar and I thought it would help a lot with the review if you could just explain them with the reason of doing it and how it was done. Thanks!
First off, nice one on hx711py, it was a great jumping off point for me. And was a primary inspiration for my choice of an HX711 for my project.
When I was waiting for delivery for my load cell I made a wheatstone bridge with a variable resister so I could work with the HX711. This is when I noticed weird discontinuities in the output from HX711.
I write device drivers for a living, so some bit wrangling isn't a problem and it was easier to reimplement the bit reading step by step to figure out what was going on than debug the numpy code.
I don't think the Most Significant Byte and / Bit ordering is needed, but I left it in to keep compatibility with the original class interface. When you lose bits, or move the order of bits in twos complement numbers you can get weird effects that depend on the sample values your getting. I think this is why it looked to you like the bytes were out of order.
The merge request makes things look way more complicated than it is. Here's the basic flow:
read_long() calls readRawBytes() calls which calls readNextByte() which calls readNextBit().
Starting from readNextBit():
If we look at the data sheet we can see DOUT with be read 1 microsecond after PD_SCK has a rising edge. The original HX711PY method reads the bit immediately after raising PD_SCK:
GPIO.output(self.PD_SCK, True) dataBits[j][i] = GPIO.input(self.DOUT) GPIO.output(self.PD_SCK, False)
We'd probably always get away with it, but it's safer to read DOUT after lowering PD_SCK, when we are guaranteed that DOUT will be stable. So that's what I do in readNextBit().
readNextByte() simply reads calls readNextBit() 8 times, by doing this at the byte level, instead of pulling in all 24 bits it's easy to control how we'll order the bits in each byte.
readRawBytes() simply calls readNextByte() three times which allows easy ordering of the bits at the byte level. The appropriate amount of extra bits that have to be read to set the channel/gain are then read out and thrown away with readNextBit().
read_long() calls readRawBytes() then uses convertFromTwosComplement24bit() to get the correct signed value.
Other areas:
Powering up: I reduced the delay from 400miliseconds to the 100microseconds specified by the datasheet. I also added reading and throwing away a sample if the client software has specified a reading other than Channel A/gain 128, as the HX711 will always default to that on power up, and can only be set to the correct gain after reading a sample.
Averaging: I added some sorting and throwing away for 20% of outlying samples, I have found that the HX711/load cell combination I have is subject to noise. This is largely redundant with the addition of read_median() which wasn't there when I forked hx711py. I did change read_median() to calculate the median with built in sort() instead of using numpy.
Thread safing: I added a threading lock, so that more than one thread can safely read from the HX711 class. Before, if two competing threads were trying to read from the HX711 they would corrupt each others sample readings, and probably the successive sample, too. Now if two threads try to access the HX711 simultaneously the second will block safely, and wait its turn.
Exceptions: I added some input verification and the raising of exceptions on bad input parameters.
Cheers, Gordon
On Tue, Feb 12, 2019 at 4:29 PM tatobari notifications@github.com wrote:
Great! For example, I've been wanting to remove numpy. Shouldn't be much of a problem. It's just a matter of shifting bits. However, all the other functionalities are mainly out of my radar and I thought it would help a lot with the review if you could just explain them with the reason of doing it and how it was done. Thanks!
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tatobari/hx711py/pull/19#issuecomment-462830075, or mute the thread https://github.com/notifications/unsubscribe-auth/ApBt4cIwrIAzbrTVkm2Ou11hrn1tDgFMks5vMuvwgaJpZM4ay2dg .
Can't believe I'm talking to someone who develops drivers. I've quickly read your message. Looks good for me. I'll test it out and probably merge directly.
However, it could take me a couple days cause I'm moving and I have all my electronics in boxes ready to me moved to the new apartment. Next week, probably.
Sorry for my late response.
I had a few minutes to spare, and, yes, this:
GPIO.output(self.PD_SCK, True)
dataBits[j][i] = GPIO.input(self.DOUT)
GPIO.output(self.PD_SCK, False)
I shouldn't be allowed to code.
A'right! Awesome job. Thanks a lot @slabula. Merging now. We're gonna need to update the readme file.
@slabula I have a small library for HX711 made in C++. I'm using it with an ATTiny841.
I realised that the look while not self.is_ready():
is not a good idea beacuse it halts the process while the HX711 is holding the DOUT
line HIGH
because it is reading the value from the sensor. This can take up to about 90ms at 10SPS.
On my little C++ library, I did the following (I'm sure you do read C++):
long read(){
while ( !isReady() );
return readNow();
}
long readNow(){
if ( !isReady() ) return 0; // this "return 0" could be replaced with something more appropriate
// ... here goes all the HX711 protocol code;
return value;
}
So, read()
can still be used, which implies waiting until the HX711 is ready every time, which would be relatively ok for cases in which the readings are on demand.
However, in case speed is important because there are other functionalities running, an interrupt could be used to trigger the readNow()
function whenever the DOUT
line goes LOW
, provided the interrupt is deactivated while reading the HX711 and reactivated once it has been read.
Hope my explanation makes sense. Would implementing this here be worth it? What do you think?
I'm asking you because you develop drivers for a living and I'm just an amateur.
Added thread safing. Fixed issues interpreting binary values. Removed dependancy on numpy.