Conversation
| - pip install -U platformio | ||
|
|
||
| script: | ||
| - platformio ci --lib="." --board=uno --board=megaatmega2560 No newline at end of file |
There was a problem hiding this comment.
travis is not used anymore, please move to github actions
There was a problem hiding this comment.
Oops, wasn't expecting anyone else to find this so fast haha, mostly opened this PR so I could review the AI's work, working on that right now!
| void TMP119::read() { | ||
| // The TMP119 powers up in continuous-conversion mode, so the | ||
| // temperature register always holds the most recent conversion. | ||
| _wire->beginTransmission(_address); | ||
| _wire->write(TMP119_TEMP_REG); | ||
| _wire->endTransmission(); | ||
|
|
||
| _wire->requestFrom(_address, (uint8_t)2); | ||
| int16_t raw = (_wire->read() << 8) | _wire->read(); | ||
|
|
||
| // Each LSB represents 0.0078125 deg C, data is in 2's complement format | ||
| TEMP = raw * 0.0078125f; | ||
| } | ||
|
|
||
| float TMP119::temperature() { | ||
| return TEMP; | ||
| } |
There was a problem hiding this comment.
Over having the user to call read and temperature every time that he wants a temperature value, you can just create a single function called getTemperature.
tmp119.read();
const float temp = tmp119.temperature();
// vs
const float temp = tmp119.getTemperature();
The reason behind the get prefix is to follow the other functions of the class that already start with get/set.
This will also make the code simpler, since you can remove a function and also a private variable.
There was a problem hiding this comment.
I'm thinking I might leave this one as-is. It's more consistent with the old library, and some other sensor libraries I've seen. I'm torn because the simplicity of one function is nice but I do like that currently tmp119.temperature(); doesn't require an I2C call and you can re-use it a bunch of times in place of needing to declare your own temperature variable.
| typedef enum { | ||
| TMP119_DELAY_0_MS, | ||
| TMP119_DELAY_125_MS, | ||
| TMP119_DELAY_250_MS, | ||
| TMP119_DELAY_500_MS, | ||
| TMP119_DELAY_1000_MS, // power-on default | ||
| TMP119_DELAY_4000_MS, | ||
| TMP119_DELAY_8000_MS, | ||
| TMP119_DELAY_16000_MS, | ||
| } tmp119_delay_t; |
There was a problem hiding this comment.
As the comment says.
/** Minimum standby delay between conversions in continuous-conversion mode
* (CONV[2:0], configuration register bits 9:7). This sets the standby time;
* the total conversion cycle time also depends on the averaging setting (see
* datasheet Table 8-6).
And in the datasheet:
With the default register value of:
0b00'100'01'0'0'0'0'0 = 220h
│││ ││ │ │ │ │ └ ---
│││ ││ │ │ │ └── Soft_Reset
│││ ││ │ │ └──── DR/Alert
│││ ││ │ └────── POL (ALERT pin polarity bit)
│││ ││ └──────── T/nA (Therm/alert mode select)
│││ ││
│││ └┴────────── AVG (0b01: 8 Averaged conversions)
└┴┴───────────── CONV (1s with AVG 01)
It's possible to validate the 1s as the default value indeed, but..
The current names of tmp119_delay_t are not correct. First, there is no configuration that matches the 0ms delay, and the standard is AVG 0b01, that results CONV[2:0] 0b000 as 125ms that's equal to CONV[2:0] 0b001.
There are two possible options IMO, one is to keep the current approach and having a different name for tmp119_delay_t values, maybe..
TMP119_DELAY_000
TMP119_DELAY_001
...This is not clear for the user sadly.. but at least is clear the values, and the user can use it based in the table in the datasheet...
The second approach is better I believe, it requires as to read the AVG and set the delay based in desired time.
typedef enum {
TMP119_DELAY_15_5_MS, // <-- Note this new name, that follows CONV 0b000 and AVG 0b00
TMP119_DELAY_125_MS,
TMP119_DELAY_250_MS,
TMP119_DELAY_500_MS,
TMP119_DELAY_1000_MS, // power-on default
TMP119_DELAY_4000_MS,
TMP119_DELAY_8000_MS,
TMP119_DELAY_16000_MS,
} tmp119_delay_t;
...
bool TMP119::setReadDelay(tmp119_delay_t delay) {
uint16_t config = getConfig();
uint8_t avg = (config & TMP119_AVG_MASK) >> TMP119_AVG_SHIFT;
switch(avg) {
case 0b00:
// Accept all delays
break;
case 0b01:
if(delay < TMP119_DELAY_125_MS) {
return false;
}
break;
case 0b10:
if(delay < TMP119_DELAY_500_MS) {
return false;
}
break;
case 0b11:
if(delay < TMP119_DELAY_1000_MS) {
return false;
}
break;
};
config = (config & ~TMP119_CONV_MASK) | ((uint16_t)delay << TMP119_CONV_SHIFT);
return setConfig(config);
}As you can see, the setReadDelay was modified to ensure that the correct parameter is being used and return false to invalid parameters.
Note: The code may be wrong, I just wrote it from the top of my head and looking at the datasheet.
There was a problem hiding this comment.
This is a great point, personally I prefer that the function directly sets the register (since this proposed function would cause issues if you try to set the delay before setting the averaging) but I've re-named some things and added a bunch more comments that make it more clear how this timing actually works without needing to read the datasheet.
| low-noise readings. | ||
|
|
||
| ------------------------------- | ||
| The MIT License (MIT) |
There was a problem hiding this comment.
Since the license is already in the repository (LICENSE), it indicates that all the files in the same repository are already in the same license.
The license header can be added when different licenses are in play in the same repository and a license for a file does not follow the main repository license file (LICENSE).
If you really want to have a license header, I recommend using SPDX-License-Identifier.
There was a problem hiding this comment.
Sweet yeah I'll remove, old carry over from the TSYS01 library
No description provided.