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

Temp sensors improvements #4282

Merged

Conversation

Projects
None yet
6 participants
@shellixyz
Copy link
Collaborator

commented Jan 28, 2019

Closes #3844

  • Add 1-Wire support through the DS2482 IF chip (new driver)
  • Add driver for the DS18B20 1-Wire temperature sensor
  • Support for multiple LM75 sensors
  • Support for the DS18B20 sensors
  • Add CLI command temp_sensor to configure sensors
  • Logging of all temperature sensor values
  • Display of all temperature sensor values on the OSD with support for alarms
  • New MSP messages to query and set sensor configuration and read temperatures
  • Enable 1-Wire and DS18B20 temp sensors on all F4 and F7 targets
  • Up to 8 temperature sensors supported

Syntax of the temp_sensor command:

  • temp_sensor reset to reset the temperature sensor configuration
  • temp_sensor index type address alarm_min alarm_max label. Max label length 4 chars. Type: 0=NONE, 1=LM75, 2=DS18B20

Configurator part: iNavFlight/inav-configurator#670

@shellixyz shellixyz force-pushed the shellixyz:temp_sensors_improvements branch 3 times, most recently from 2b2b3b0 to 59f64b2 Jan 28, 2019

@ghost

This comment has been minimized.

Copy link

commented Jan 28, 2019

Your a work horse shelli. ; )

These are some idea's to define the External temperature sensor from the Internal barometer temperature reading.
The EXT or Thermometer is the most probable. The others are just descriptive of the things that it will be used for. Likely a waste of 7456 memory, adding them all.

thermo
vtx
battery
esc
external temp sensor
motor

Temp sensor devices.zip

@shellixyz shellixyz force-pushed the shellixyz:temp_sensors_improvements branch from 59f64b2 to 00d484e Jan 28, 2019

@wx4cb

This comment has been minimized.

Copy link

commented Jan 28, 2019

@shellixyz

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 28, 2019

@kardon18 Good idea it will help save space on the screen by replacing labels. Could you make one for MPU and baro temperature ?

@ghost

This comment has been minimized.

Copy link

commented Jan 29, 2019

@shellixyz Good idea it will help save space on the screen by replacing labels. Could you make one for MPU and baro temperature ?

I was hoping to have a descriptive icon for the temperature of the device we are measuring. That way we can look at a glance.
I'm glad you included an ALARM for over temperature in the code 👍

When you stated MPU. I assume you were refering to the Accelerometer/Gyro. Not "Micro Processor Unit". Being that both can use internal components to read temperature.
That being said. I wrote MPU as IMU. Because its impossible to fit "MPU" across 12 pixels. Due to the width of the 'M". But with 'I" it will fit.

Hope that's okay. If not, let me know.

barometer temperature
imu temp

Internal temp sensor.zip

@wx4cb

This comment has been minimized.

Copy link

commented Jan 29, 2019

@shellixyz

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 29, 2019

@kardon18 MPU is for Motion Processing Unit (gyro + accelerometer). IMU is good 👍. Thank you the new symbols will fit the purpose.

@wx4cb On top of the possibility to connect dedicated temperature sensor we are reading the gyro/acc chip and baro temperature. We are not reading MCU temp. Maybe it is possible, I don't know.

@shellixyz shellixyz force-pushed the shellixyz:temp_sensors_improvements branch from 00d484e to 3a83118 Jan 29, 2019

@fiam

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

We could have a generic temperature character on page 1 and specific ones on page 2.

@ghost

This comment has been minimized.

Copy link

commented Jan 30, 2019

We could have a generic temperature character on page 1 and specific ones on page 2.

I suppose that it depends on how many sensors the model will have on it.
e.g. One user may want to read the temperature of 4 motors or ESC's. While another may what to read the temperature of 1 ESC or a VTX.

What do you guys have in mind for the selection ability?
CLI command? Or drop down box's in the configurator?

The user would need the ability to select -

  1. How many sensors they are using.
  2. Then define a label for each.
  3. Set an alarm preset for each corresponding sensors label.
@shellixyz

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 30, 2019

I was just going to add an option to the temp_sensor command to chose between a char to display and the label. Take a look at the description I added the current syntax for the command.

@ghost

This comment has been minimized.

Copy link

commented Jan 30, 2019

I was just going to add an option to the temp_sensor command to chose between a char to display and the label. Take a look at the description I added the current syntax for the command.

I think that's perfectly fine. 👍
In reality, most users will be more than happy to have one external temp sensor. With the ability to choose a sensor label, and give it an icon for recognition.

As you spoke of earlier. To add more sensors, it would require the use of a DS2482-100, I2C interface chip. So most users wouldn't go to that much effort. Although. I do remember reading on the RCG ET Vector forum. Many people requested the desire of an expansion of its temperature sensor ability, out to 4 or 6. So the motor temperatures on their multirotors could be measured.

But there is away the possibility for expansion in future releases.

@shellixyz

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 30, 2019

To be honest adding a DS2482 is really simple with the help of a SO8 breakout and cheap. You can get 10 of them for $5 on AliExpress

@wx4cb

This comment has been minimized.

Copy link

commented Jan 30, 2019

@shellixyz

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 30, 2019

BLHeli32 does not monitor motors temperature. The temperature of the ESC itself yes, the motor no.

@wx4cb

This comment has been minimized.

Copy link

commented Jan 31, 2019

BLHeli32 does not monitor motors temperature. The temperature of the ESC itself yes, the motor no.

I realise that, that's what I meant, but i have no clue how you would measure the actual "motor" temp as it's kinda... spinning really fast lol :D

@shellixyz shellixyz force-pushed the shellixyz:temp_sensors_improvements branch from 3a83118 to 2ce4cb8 Jan 31, 2019

@shellixyz

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 31, 2019

What gets hot is the base not the bell. You can put a sensor in contact with the motor base.

@@ -141,8 +141,12 @@ static uint8_t dispatchMeasurementRequest(ibusAddress_t address) {
}
#endif
if (SENSOR_ADDRESS_TYPE_LOOKUP[address].value == IBUS_MEAS_VALUE_TEMPERATURE) { //BARO_TEMP\GYRO_TEMP
if (sensors(SENSOR_BARO)) return sendIbusMeasurement2(address, (uint16_t) ((DEGREES_TO_CENTIDEGREES(getCurrentTemperature()) + 50) / 10 + IBUS_TEMPERATURE_OFFSET)); //int32_t
else {
/*if (sensors(SENSOR_BARO)) return sendIbusMeasurement2(address, (uint16_t) ((DEGREES_TO_CENTIDEGREES(getCurrentTemperature()) + 50) / 10 + IBUS_TEMPERATURE_OFFSET)); //int32_t*/

This comment has been minimized.

Copy link
@hali9

hali9 Feb 1, 2019

Contributor

If I can suggest, it can be:

int16_t temperature;
if (sensors(SENSOR_BARO)) {
    getBaroTemperature(&temperature);
} else {
    getMPUTemperature(&temperature);
}
return sendIbusMeasurement2(address, (uint16_t) ((temperature + 50) / 10  + IBUS_TEMPERATURE_OFFSET));

This comment has been minimized.

Copy link
@shellixyz

shellixyz Feb 2, 2019

Author Collaborator

Why not, done, it now reports baro temp if available, IMU temp otherwise

@shellixyz shellixyz force-pushed the shellixyz:temp_sensors_improvements branch 6 times, most recently from 28cec6f to 11bda67 Feb 2, 2019

}
int16_t temperature;
const bool temp_valid = sensors(SENSOR_BARO) ? getBaroTemperature(&temperature) : getIMUTemperature(&temperature);
if (!temp_valid || (temperature < -50)) temperature = -50; // Minimum reported temperature is -5°C

This comment has been minimized.

Copy link
@hali9

hali9 Feb 2, 2019

Contributor

Not realy, ibus temperatue sensor minimum is 40 deg.
0=-40.0 C, 400=0.0 C, 65535=6513.5 C
so proposes change -50 to -400

This comment has been minimized.

Copy link
@shellixyz

shellixyz Feb 3, 2019

Author Collaborator

True, I forgot about the offset

int16_t temperature;
const bool temp_valid = sensors(SENSOR_BARO) ? getBaroTemperature(&temperature) : getIMUTemperature(&temperature);
if (!temp_valid || (temperature < -50)) temperature = -50; // Minimum reported temperature is -5°C
return sendIbusMeasurement2(address, (uint16_t)((temperature + 50) / 10 + IBUS_TEMPERATURE_OFFSET));

This comment has been minimized.

Copy link
@hali9

hali9 Feb 2, 2019

Contributor

if getBaroTemperature and getIMUTemperature return temperature in decidegree (0.1) no need to be calculated
Just (uint16_t)(temperature + IBUS_TEMPERATURE_OFFSET)

This comment has been minimized.

Copy link
@shellixyz

shellixyz Feb 2, 2019

Author Collaborator

Are you sure about that ?

This comment has been minimized.

Copy link
@hali9

hali9 Feb 2, 2019

Contributor

Yes I sure.

This comment has been minimized.

Copy link
@shellixyz

shellixyz Feb 3, 2019

Author Collaborator

It doesn't match the old code:

return sendIbusMeasurement2(address, (uint16_t) ((DEGREES_TO_CENTIDEGREES(getCurrentTemperature()) + 50) / 10  + IBUS_TEMPERATURE_OFFSET));

This comment has been minimized.

Copy link
@hali9

hali9 Feb 3, 2019

Contributor

Yesterday I compiled and uploaded the code from Your branch with minimum -400 and (uint16_t)(temperature + IBUS_TEMPERATURE_OFFSET) and got the correct temperature values for both IMU and baro. For baro get -40.0 for a first second after power up.

This comment has been minimized.

Copy link
@shellixyz

shellixyz Feb 7, 2019

Author Collaborator

Nice. I didn't touch the code reading the baro temperature. Not a big issue I think

@shellixyz shellixyz force-pushed the shellixyz:temp_sensors_improvements branch from 342d52e to 8e12f12 Feb 2, 2019

Show resolved Hide resolved src/main/blackbox/blackbox.c
_1WireDev_t ds2482Dev;
#endif

void _1WireInit(void)

This comment has been minimized.

Copy link
@digitalentity

digitalentity Feb 2, 2019

Member

Our convention is to use chip name as a file name and function name prefix.

{
bool parasitic_power = ds18b20_parasitic_powered_present(_1WireDev);
if (parasitic_power) return false;
bool ack = ds2482_1wire_reset_and_skip_rom(_1WireDev);

This comment has been minimized.

Copy link
@digitalentity

digitalentity Feb 2, 2019

Member

I don't like this explicit dependency. We should either make 1-wire driver abstracted and make ds18b20 call high-level 1-wire read/write functions or merge the ds18b20 with ds2482.

return ds2482_1wire_write_byte(_1WireDev, DS18B20_START_CONVERSION_CMD);
}

bool ds18b20_wait_for_conversion()

This comment has been minimized.

Copy link
@digitalentity

digitalentity Feb 2, 2019

Member

In functions waiting for external sensor to complete the operation protothreads play quite nicely.

This comment has been minimized.

Copy link
@shellixyz

shellixyz Feb 3, 2019

Author Collaborator

Yes I need to take a look into it because even though everything works fine on the bench after checking the protocol timing reading the sensor values takes too much time. The reading process will need to yield before all the sensors are read or even during addressing/reading. 1-Wire is pretty slow. Only addressing the sensor takes 7.7ms. Reading the temperature value then takes 8.1ms. So we are talking a bit less than 16ms per sensor to read back the temperature value. Fortunately we can send the "start conversion" to all the sensors at once and that only takes ~2.2ms.

This comment has been minimized.

Copy link
@digitalentity

digitalentity Feb 5, 2019

Member

Still, even 2.2ms is too much to do in one iteration. I think we should really think into making this ptThreaded eventually

This comment has been minimized.

Copy link
@shellixyz

shellixyz Feb 7, 2019

Author Collaborator

Resetting the 1-Wire bus (needed before every command) is taking 1345µs in itself so it looks like we will need to. We need to send the reset command (54µs) then yield and the next iterations check if the bus has been reset (54µs) then send a new command

extern _1WireDev_t ds2482Dev;
#endif

void _1WireInit(void);

This comment has been minimized.

Copy link
@digitalentity

digitalentity Feb 2, 2019

Member

OneWireInit() maybe?

This comment has been minimized.

Copy link
@shellixyz

shellixyz Feb 3, 2019

Author Collaborator

Ok yes I don't really like starting names with _

This comment has been minimized.

Copy link
@shellixyz

shellixyz Feb 7, 2019

Author Collaborator

Fixed

@shellixyz shellixyz force-pushed the shellixyz:temp_sensors_improvements branch from 8e12f12 to b0cca40 Feb 3, 2019

@shellixyz shellixyz added this to the 2.1 milestone Feb 3, 2019

@wx4cb

This comment has been minimized.

Copy link

commented Feb 3, 2019

@shellixyz

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 3, 2019

@wx4cb What are talking about ?

@wx4cb

This comment has been minimized.

Copy link

commented Feb 3, 2019

@shellixyz i was replying to a comment that i got theough email about -40 for a couple seconds at startup. Dunno why it appeared here im guessing it was in another thread

@shellixyz shellixyz force-pushed the shellixyz:temp_sensors_improvements branch from 07d10e0 to 63a0a8f Feb 7, 2019

shellixyz added some commits Jan 28, 2019

Improve temperature framework
- Support for multiple LM75 sensors
- Support for the DS18B20 sensors
- CLI command temp_sensor to configure sensors
- Logging of all temperature sensor values
- Display of all temperature sensor values on the OSD with support for alarms
- New MSP messages to query and set sensor configuration and read temperatures

@shellixyz shellixyz force-pushed the shellixyz:temp_sensors_improvements branch from 63a0a8f to 4c33402 Feb 7, 2019

shellixyz added some commits Feb 8, 2019

@digitalentity
Copy link
Member

left a comment

We should have an action item to refactor it further, but in general LGTM.

@digitalentity digitalentity merged commit 1dbc1a4 into iNavFlight:development Feb 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ghost

This comment has been minimized.

Copy link

commented Feb 8, 2019

@shellixyz I was going to wire up a few DS18B20's, in anticipation of the 2.1 release.
What do you have in mind for the 4.7K pull-up resistor. It will need to be an external component, when being used with a DS2482.
But will it use a pull-up resistor in the controller, when using a single sensor?

@teckel12

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2019

@kardon18 You only need one pull-up resistor. Some FC already include a pull-up resistor on the i2c pin. I'd try it without the pull-up resistor and then add one if needed.

A resistor only costs about a cent, and having a few common sizes isn't a bad idea. You could probably remove one from an old piece of electronics even. It doesn't need to be 4.7k, 1k to 10k would probably work just as well.

I need to see if the F4/F7 has internal pull-up resistors. Many microcontrollers do. I know the Arduino's Amtel ATmega328 microprocessor has the ability to activate a pull-up resistor on each pin. If the F4/F7 also has this, it could be activated via code.

@ghost

This comment has been minimized.

Copy link

commented Feb 9, 2019

I need to see if the F4/F7 does have internal pull-up resistors. Many microcontrollers do. If the F4/F7 also has this, it could be activated via code.

That's what I was wondering. If the F4/F7 does have internal pull-up resistors. Will they be used.

If not. I'll just solder a 0603 SMD resistor across the VCC and Data pins on the DS18D20. To save space and keep it tidy.

@teckel12

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2019

@kardon18 It does appear that the F4/F7 have internal pull-up resistors. As I would classify temp readings as non-critical, using the known weak internal pull-up resistors would probably be fine (although would need to be activated via software, which I don't believe is happening).

You may want to see if your FC design already includes an external resistor. Or, if you have something else already on i2c it probably has a pull-up resistor of it's own, so you don't need another.

I'd try it without and only add the resistor if it doesn't work or is unreliable.

@shellixyz

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 10, 2019

@kardon18 On the I²C side some FC have pull-up resisors, some don't, you have to check on the documentation or with a multimeter. I know Matek FCs already have pull-up resistors on the I²C bus, Omnibus FCs don't. If your FC doesn't have pull-up resistors you have to add them. One for the SCL line and one for SDA to 3.3V, value between 1k and 4.7k. Higher values should work but are not recommended unless the I²C bus is very short. On the 1-Wire side you need a pull-up resistor too, same range of values between VCC and the data line. For now the 1-wire parasitic power mode isn't supported you need to wire VCC to the sensors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.