From e27c424b7b01a7027ef6fde03461b7659dafb486 Mon Sep 17 00:00:00 2001 From: Michael Traver Date: Tue, 2 Apr 2019 07:29:53 -0700 Subject: [PATCH] mcp9808: Fix conversions between temperature and bits (#403) The datasheet (http://ww1.microchip.com/downloads/en/DeviceDoc/25095A.pdf) says that negative temperatures are stored in two's complement. See page 22 for alert registers and pages 24-25 for the ambient temperature register. However, the code currently assumes negative values are stored in a sign-and-magnitude representation. Fix this to handle two's complement values correctly. For register bits to temperature conversion this implementation follows the datasheet, but the code in the datasheet isn't quite right -- it should be `temp - 256` instead of `256 - temp` -- but the fact that the 256 figure is in Microchip's code backs up the idea stated in the text that negative values are in two's complement. Others seem to agree that this is correct: https://electronics.stackexchange.com/a/244826 For temperature to bits conversion this implementation relies on the fact that Go also uses two's complement. I've added a long comment in the code about this. --- experimental/devices/mcp9808/mcp9808.go | 21 ++++++------- experimental/devices/mcp9808/mcp9808_test.go | 31 +++++++++++++------- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/experimental/devices/mcp9808/mcp9808.go b/experimental/devices/mcp9808/mcp9808.go index e4c20db..cb45c81 100644 --- a/experimental/devices/mcp9808/mcp9808.go +++ b/experimental/devices/mcp9808/mcp9808.go @@ -266,8 +266,8 @@ func (d *Dev) readTemperature() (physic.Temperature, uint8, error) { // Convert to physic.Temperature 0.0625°C per bit t := physic.Temperature(tbits&0x0FFF) * 62500 * physic.MicroKelvin if tbits&0x1000 > 0 { - // Check for bit sign. - t = -t + // Check for sign bit. + t -= 256 * physic.Celsius } t += physic.ZeroCelsius return t, uint8(tbits>>8) & 0xe0, nil @@ -386,7 +386,7 @@ func alertBitsToTemperature(b uint16) physic.Temperature { b = (b >> 2) & 0x07FF t := physic.Temperature(b&0x03FF) * 250 * physic.MilliKelvin if b&0x400 > 0 { - t = -t + t -= 256 * physic.Celsius } t += physic.ZeroCelsius return t @@ -403,13 +403,14 @@ func alertTemperatureToBits(t physic.Temperature) (uint16, error) { // 0.25°C per bit. t /= 250 * physic.MilliKelvin - var bits uint16 - if t < 0 { - t = -t - bits |= 0x400 - } - bits |= uint16(t) - bits = bits << 2 + // We don't need to explicitly handle negative temperatures because both Go and the MCP9808 + // store negative values using two's complement. We can rely on Go's implementation since + // we know that the bits of a negative value are already in two's complement, implying that + // the sign bit will already be set to 1 due to the range check. We need to be sure to mask + // off the 3 most significant bits after shifting though: they will all be set to 1 if the + // value is negative. Also mask off the 2 least significant bits. While not strictly necessary, + // the MCP9808 doesn't use them. + bits := (uint16(t) << 2) & 0x1ffc return bits, nil } diff --git a/experimental/devices/mcp9808/mcp9808_test.go b/experimental/devices/mcp9808/mcp9808_test.go index fab8198..96581ca 100644 --- a/experimental/devices/mcp9808/mcp9808_test.go +++ b/experimental/devices/mcp9808/mcp9808_test.go @@ -94,7 +94,8 @@ func TestSense(t *testing.T) { { name: "-10C", tx: []i2ctest.IO{ - {Addr: 0x18, W: []byte{temperature}, R: []byte{0x10, 0xa0}}, + // 0x1f60 is -10°C in two's complement. See page 24-25 of the datasheet. + {Addr: 0x18, W: []byte{temperature}, R: []byte{0x1f, 0x60}}, }, enabled: true, want: physic.ZeroCelsius - 10*physic.Kelvin, @@ -284,7 +285,8 @@ func TestSenseTemp(t *testing.T) { { name: "-10C", tx: []i2ctest.IO{ - {Addr: 0x18, W: []byte{temperature}, R: []byte{0x10, 0xa0}}, + // 0x1f60 is -10°C in two's complement. See page 24-25 of the datasheet. + {Addr: 0x18, W: []byte{temperature}, R: []byte{0x1f, 0x60}}, }, want: physic.ZeroCelsius - 10*physic.Kelvin, err: nil, @@ -356,7 +358,9 @@ func TestSenseWithAlerts(t *testing.T) { {Addr: 0x18, W: []byte{critAlert, 0x00, 0xa0}, R: nil}, {Addr: 0x18, W: []byte{upperAlert, 0x00, 0x50}, R: nil}, {Addr: 0x18, W: []byte{lowerAlert, 0x00, 0x00}, R: nil}, - {Addr: 0x18, W: []byte{temperature}, R: []byte{0x30, 0x10}}, + // 0x3ff0 is -1°C in two's complement with the lower alert bit set (bit 13). + // See page 24-25 of the datasheet. + {Addr: 0x18, W: []byte{temperature}, R: []byte{0x3f, 0xf0}}, {Addr: 0x18, W: []byte{lowerAlert}, R: []byte{0x00, 0x00}}, }, err: nil, @@ -487,7 +491,9 @@ func TestSenseWithAlerts(t *testing.T) { {Addr: 0x18, W: []byte{critAlert, 0x00, 0xa0}, R: nil}, {Addr: 0x18, W: []byte{upperAlert, 0x00, 0x50}, R: nil}, {Addr: 0x18, W: []byte{lowerAlert, 0x00, 0x00}, R: nil}, - {Addr: 0x18, W: []byte{temperature}, R: []byte{0x30, 0x10}}, + // 0x3ff0 is -1°C in two's complement with the lower alert bit set (bit 13). + // See page 24-25 of the datasheet. + {Addr: 0x18, W: []byte{temperature}, R: []byte{0x3f, 0xf0}}, }, err: errReadLowerAlert, }, @@ -897,7 +903,9 @@ func Test_alertBitsToTemperature(t *testing.T) { }{ {"0°C", 0x0000, physic.ZeroCelsius}, {"0.25°C", 0x0004, physic.ZeroCelsius + 250*physic.MilliKelvin}, - {"-0.25°C", 0x1004, physic.ZeroCelsius - 250*physic.MilliKelvin}, + // Negative values are in two's complement. See page 22 of the datasheet. + {"-0.25°C", 0x1ffc, physic.ZeroCelsius - 250*physic.MilliKelvin}, + {"-39.75°C", 0x1d84, physic.ZeroCelsius - 39750*physic.MilliKelvin}, } for _, tt := range tests { @@ -907,7 +915,7 @@ func Test_alertBitsToTemperature(t *testing.T) { } } -func Test_temperatureToAlertBits(t *testing.T) { +func Test_alertTemperatureToBits(t *testing.T) { succeeds := []struct { name string alert physic.Temperature @@ -915,9 +923,10 @@ func Test_temperatureToAlertBits(t *testing.T) { }{ {"0°C", physic.ZeroCelsius, 0x0000}, {"0.25°C", physic.ZeroCelsius + 250*physic.MilliKelvin, 0x0004}, - {"-0.25°C", physic.ZeroCelsius - 250*physic.MilliKelvin, 0x1004}, {"124.75°C", physic.ZeroCelsius + 124750*physic.MilliKelvin, 0x07cc}, - {"-39.75°C", physic.ZeroCelsius - 39750*physic.MilliKelvin, 0x127c}, + // Negative values are in two's complement. See page 22 of the datasheet. + {"-0.25°C", physic.ZeroCelsius - 250*physic.MilliKelvin, 0x1ffc}, + {"-39.75°C", physic.ZeroCelsius - 39750*physic.MilliKelvin, 0x1d84}, } fails := []struct { @@ -932,16 +941,16 @@ func Test_temperatureToAlertBits(t *testing.T) { for _, tt := range succeeds { got, err := alertTemperatureToBits(tt.alert) if got != tt.want && err == nil { - t.Errorf("alertBitsToTemperature(%s) = %x, want %x", tt.name, got, tt.want) + t.Errorf("alertTemperatureToBits(%s) = %x, want %x", tt.name, got, tt.want) } if err != nil { - t.Errorf("alertBitsToTemperature(%s) got unexpected error: %v", tt.name, err) + t.Errorf("alertTemperatureToBits(%s) got unexpected error: %v", tt.name, err) } } for _, tt := range fails { if _, err := alertTemperatureToBits(tt.alert); err == nil { - t.Errorf("alertBitsToTemperature(%s) expected error %v", tt.name, tt.want) + t.Errorf("alertTemperatureToBits(%s) expected error %v", tt.name, tt.want) } } }