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

@@ -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.
+		// Check for sign bit.
-		t = -t
+		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
+	// We don't need to explicitly handle negative temperatures because both Go and the MCP9808
-	if t < 0 {
+	// store negative values using two's complement. We can rely on Go's implementation since
-		t = -t
+	// we know that the bits of a negative value are already in two's complement, implying that
-		bits |= 0x400
+	// 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
-	bits |= uint16(t)
+	// value is negative. Also mask off the 2 least significant bits. While not strictly necessary,
-	bits = bits << 2
+	// the MCP9808 doesn't use them.
+	bits := (uint16(t) << 2) & 0x1ffc
 	return bits, nil
 }
 

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)
 		}
 	}
 }