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

Clock wrap not catered for and other issues from a code read through #51

Closed
MalcolmBoura opened this issue Oct 18, 2020 · 7 comments
Closed

Comments

@MalcolmBoura
Copy link
Contributor

In HX711_ADC::startMultiple(unsigned int t) appears the line

if(millis() < startMultipleTimeStamp + startMultipleWaitTime)

which will fail to work as expected if startMultipleTimeStamp + startMultipleWaitTime wraps around the clock. The fix is to work with durations instead of times.

if ( millis() - startMultipleTimeStamp > startMultipleWaitTime)

See https://www.norwegiancreations.com/2018/10/arduino-tutorial-avoiding-the-overflow-issue-when-using-millis-and-micros/
I will create a PR but it may be a few days before I can do that.

@MalcolmBoura
Copy link
Contributor Author

MalcolmBoura commented Oct 18, 2020

Likewise in int HX711_ADC::startMultiple(unsigned int t, bool dotare)
and I suspect a lot of other places! Yes, further down the same functions for a start.

@MalcolmBoura MalcolmBoura changed the title Clock wrap not catered for Clock wrap not catered for and other issues from a code read through Oct 18, 2020
@MalcolmBoura
Copy link
Contributor Author

uint8_t HX711_ADC::conversion24bit()
unsigned long data = 0;
if ((data < 0x000000) || (data > 0xFFFFFF))
the first part can never be true since data is unsigned.

@MalcolmBoura
Copy link
Contributor Author

MalcolmBoura commented Oct 18, 2020

data = data ^ 0x800000; // if the 24th bit is '1', change 24th bit to 0
it will also change the 24th bit from '0' to '1'. Is that intended? I think it is in which case the comment is misleading.

@MalcolmBoura
Copy link
Contributor Author

data = data << 1;
if (dout)
{
data++;
}
The first line of that can be written data <<= 1
Depending on how smart the compiler is, data |= 1 may be slightly faster than data++.

I would probably have written this as:
data = (data << 1) | (dout == HIGH ? 1 : 0);
I can't see a value specified for HIGH and LOW in the Arduino docs. If specified to be 1 and 0 then this could be reduced to
data = (data << 1) | dout;

@MalcolmBoura
Copy link
Contributor Author

A lot of this, the low/high value code, can be compiled out when not needed and the condition removed from the subtractions.
long HX711_ADC::smoothedData()
{
long data = 0;
long L = 0xFFFFFF;
long H = 0x00;
//for (uint8_t r = 0; r < DATA_SET; r++) {
for (uint8_t r = 0; r < (samplesInUse + IGN_HIGH_SAMPLE + IGN_LOW_SAMPLE); r++)
{
#if IGN_LOW_SAMPLE
if (L > dataSampleSet[r]) L = dataSampleSet[r]; // find lowest value
#endif
#if IGN_HIGH_SAMPLE
if (H < dataSampleSet[r]) H = dataSampleSet[r]; // find highest value
#endif
data += dataSampleSet[r];
}
#if IGN_LOW_SAMPLE
data -= L; //remove lowest value
#endif
#if IGN_HIGH_SAMPLE
data -= H; //remove highest value
#endif
return data;
}

@MalcolmBoura
Copy link
Contributor Author

smoothedData()
I am surprised that it does not do the >> divBit

@olkal
Copy link
Owner

olkal commented Oct 19, 2020

Many thanks Malcolm! All your suggestions is now implemented in 1.2.4

@olkal olkal closed this as completed Oct 19, 2020
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

No branches or pull requests

2 participants