fix(cantool)!: generated messages' CopyFrom preserves invalid signals

Previously we used the setters and getters in the CopyFrom method. The
getters and setters both caps a signal to be within [min, max] range.

This meant that if we used CopyFrom from a message which contained an
out-of-bounds signal the new value would not keep the same signal
value.

This commit updates the CopyFrom implementation to instead unmarshal
the generated CAN frame from the original message.

BREAKING CHANGE: This changes CopyFrom to accept a message instead of
a message reader to copy from, however mostly test code should be
affected.

internal/generate/compile_test.go

@@ -2,9 +2,11 @@ package generate
 
 import (
 	"os"
+	"reflect"
 	"testing"
 	"time"
 
+	"go.einride.tech/can"
 	"go.einride.tech/can/pkg/descriptor"
 	examplecan "go.einride.tech/can/testdata/gen/go/example"
 	"gotest.tools/v3/assert"
@@ -376,3 +378,28 @@ func TestCompile_Float32InvalidSignalLengthWarningExpected(t *testing.T) {
 	// We expect one warning for incorrect signal length in declaration of float32 signal
 	assert.Equal(t, len(result.Warnings), 1)
 }
+
+func Test_CopyFrom_PreservesOutOfRangeValues(t *testing.T) {
+	descriptor := examplecan.Messages().MotorCommand
+	frame := can.Frame{
+		ID:         descriptor.ID,
+		Length:     descriptor.Length,
+		IsExtended: descriptor.IsExtended,
+	}
+	// 0xF is 15, but max is set to 9
+	descriptor.Drive.MarshalUnsigned(&frame.Data, 0xF)
+	// Unmarshal out of bounds value
+	original := examplecan.NewMotorCommand()
+	if err := original.UnmarshalFrame(frame); err != nil {
+		t.Errorf("Failed to unmarshal frame: %v", err)
+	}
+	// When we CopyFrom original message to m2
+	m2 := examplecan.NewMotorCommand().CopyFrom(original)
+	// Then we expect the messages and the frames to be identical
+	if !reflect.DeepEqual(m2, original) {
+		t.Errorf("Expected new message (%v) and original (%v) to be identical", m2, original)
+	}
+	if m2.Frame() != original.Frame() {
+		t.Errorf("Expected frames of messages to be identical (%v != %v)", m2.Frame(), original.Frame())
+	}
+}

internal/generate/file.go

@@ -198,8 +198,8 @@ func MessageType(f *File, m *descriptor.Message) {
 	f.P()
 	f.P("// ", messageWriterInterface(m), " provides write access to a ", m.Name, " message.")
 	f.P("type ", messageWriterInterface(m), " interface {")
-	f.P("// CopyFrom copies all values from ", messageReaderInterface(m), ".")
-	f.P("CopyFrom(", messageReaderInterface(m), ") *", messageStruct(m))
+	f.P("// CopyFrom copies all values from ", messageStruct(m), ".")
+	f.P("CopyFrom(*", messageStruct(m), ") *", messageStruct(m))
 	for _, s := range m.Signals {
 		if hasPhysicalRepresentation(s) {
 			f.P("// Set", s.Name, " sets the physical value of the ", s.Name, " signal.")
@@ -241,14 +241,8 @@ func MessageType(f *File, m *descriptor.Message) {
 	}
 	f.P("}")
 	f.P()
-	f.P("func (m *", messageStruct(m), ") CopyFrom(o ", messageReaderInterface(m), ") *", messageStruct(m), "{")
-	for _, s := range m.Signals {
-		if hasPhysicalRepresentation(s) {
-			f.P("m.Set", s.Name, "(o.", s.Name, "())")
-		} else {
-			f.P("m.", signalField(s), " = o.", s.Name, "()")
-		}
-	}
+	f.P("func (m *", messageStruct(m), ") CopyFrom(o *", messageStruct(m), ") *", messageStruct(m), "{")
+	f.P("_ = m.UnmarshalFrame(o.Frame())")
 	f.P("return m")
 	f.P("}")
 	f.P()

testdata/gen/go/example/example.dbc.go

@@ -40,8 +40,8 @@ type EmptyMessageReader interface {
 
 // EmptyMessageWriter provides write access to a EmptyMessage message.
 type EmptyMessageWriter interface {
-	// CopyFrom copies all values from EmptyMessageReader.
-	CopyFrom(EmptyMessageReader) *EmptyMessage
+	// CopyFrom copies all values from EmptyMessage.
+	CopyFrom(*EmptyMessage) *EmptyMessage
 }
 
 type EmptyMessage struct {
@@ -56,7 +56,8 @@ func NewEmptyMessage() *EmptyMessage {
 func (m *EmptyMessage) Reset() {
 }
 
-func (m *EmptyMessage) CopyFrom(o EmptyMessageReader) *EmptyMessage {
+func (m *EmptyMessage) CopyFrom(o *EmptyMessage) *EmptyMessage {
+	_ = m.UnmarshalFrame(o.Frame())
 	return m
 }
 
@@ -114,8 +115,8 @@ type DriverHeartbeatReader interface {
 
 // DriverHeartbeatWriter provides write access to a DriverHeartbeat message.
 type DriverHeartbeatWriter interface {
-	// CopyFrom copies all values from DriverHeartbeatReader.
-	CopyFrom(DriverHeartbeatReader) *DriverHeartbeat
+	// CopyFrom copies all values from DriverHeartbeat.
+	CopyFrom(*DriverHeartbeat) *DriverHeartbeat
 	// SetCommand sets the value of the Command signal.
 	SetCommand(DriverHeartbeat_Command) *DriverHeartbeat
 }
@@ -134,8 +135,8 @@ func (m *DriverHeartbeat) Reset() {
 	m.xxx_Command = 0
 }
 
-func (m *DriverHeartbeat) CopyFrom(o DriverHeartbeatReader) *DriverHeartbeat {
-	m.xxx_Command = o.Command()
+func (m *DriverHeartbeat) CopyFrom(o *DriverHeartbeat) *DriverHeartbeat {
+	_ = m.UnmarshalFrame(o.Frame())
 	return m
 }
 
@@ -232,8 +233,8 @@ type MotorCommandReader interface {
 
 // MotorCommandWriter provides write access to a MotorCommand message.
 type MotorCommandWriter interface {
-	// CopyFrom copies all values from MotorCommandReader.
-	CopyFrom(MotorCommandReader) *MotorCommand
+	// CopyFrom copies all values from MotorCommand.
+	CopyFrom(*MotorCommand) *MotorCommand
 	// SetSteer sets the physical value of the Steer signal.
 	SetSteer(float64) *MotorCommand
 	// SetDrive sets the physical value of the Drive signal.
@@ -256,9 +257,8 @@ func (m *MotorCommand) Reset() {
 	m.xxx_Drive = 0
 }
 
-func (m *MotorCommand) CopyFrom(o MotorCommandReader) *MotorCommand {
-	m.SetSteer(o.Steer())
-	m.SetDrive(o.Drive())
+func (m *MotorCommand) CopyFrom(o *MotorCommand) *MotorCommand {
+	_ = m.UnmarshalFrame(o.Frame())
 	return m
 }
 
@@ -356,8 +356,8 @@ type SensorSonarsReader interface {
 
 // SensorSonarsWriter provides write access to a SensorSonars message.
 type SensorSonarsWriter interface {
-	// CopyFrom copies all values from SensorSonarsReader.
-	CopyFrom(SensorSonarsReader) *SensorSonars
+	// CopyFrom copies all values from SensorSonars.
+	CopyFrom(*SensorSonars) *SensorSonars
 	// SetMux sets the value of the Mux signal.
 	SetMux(uint8) *SensorSonars
 	// SetErrCount sets the value of the ErrCount signal.
@@ -412,17 +412,8 @@ func (m *SensorSonars) Reset() {
 	m.xxx_NoFiltRear = 0
 }
 
-func (m *SensorSonars) CopyFrom(o SensorSonarsReader) *SensorSonars {
-	m.xxx_Mux = o.Mux()
-	m.xxx_ErrCount = o.ErrCount()
-	m.SetLeft(o.Left())
-	m.SetNoFiltLeft(o.NoFiltLeft())
-	m.SetMiddle(o.Middle())
-	m.SetNoFiltMiddle(o.NoFiltMiddle())
-	m.SetRight(o.Right())
-	m.SetNoFiltRight(o.NoFiltRight())
-	m.SetRear(o.Rear())
-	m.SetNoFiltRear(o.NoFiltRear())
+func (m *SensorSonars) CopyFrom(o *SensorSonars) *SensorSonars {
+	_ = m.UnmarshalFrame(o.Frame())
 	return m
 }
 
@@ -624,8 +615,8 @@ type MotorStatusReader interface {
 
 // MotorStatusWriter provides write access to a MotorStatus message.
 type MotorStatusWriter interface {
-	// CopyFrom copies all values from MotorStatusReader.
-	CopyFrom(MotorStatusReader) *MotorStatus
+	// CopyFrom copies all values from MotorStatus.
+	CopyFrom(*MotorStatus) *MotorStatus
 	// SetWheelError sets the value of the WheelError signal.
 	SetWheelError(bool) *MotorStatus
 	// SetSpeedKph sets the physical value of the SpeedKph signal.
@@ -648,9 +639,8 @@ func (m *MotorStatus) Reset() {
 	m.xxx_SpeedKph = 0
 }
 
-func (m *MotorStatus) CopyFrom(o MotorStatusReader) *MotorStatus {
-	m.xxx_WheelError = o.WheelError()
-	m.SetSpeedKph(o.SpeedKph())
+func (m *MotorStatus) CopyFrom(o *MotorStatus) *MotorStatus {
+	_ = m.UnmarshalFrame(o.Frame())
 	return m
 }
 
@@ -743,8 +733,8 @@ type IODebugReader interface {
 
 // IODebugWriter provides write access to a IODebug message.
 type IODebugWriter interface {
-	// CopyFrom copies all values from IODebugReader.
-	CopyFrom(IODebugReader) *IODebug
+	// CopyFrom copies all values from IODebug.
+	CopyFrom(*IODebug) *IODebug
 	// SetTestUnsigned sets the value of the TestUnsigned signal.
 	SetTestUnsigned(uint8) *IODebug
 	// SetTestEnum sets the value of the TestEnum signal.
@@ -786,13 +776,8 @@ func (m *IODebug) Reset() {
 	m.xxx_TestScaledEnum = 0
 }
 
-func (m *IODebug) CopyFrom(o IODebugReader) *IODebug {
-	m.xxx_TestUnsigned = o.TestUnsigned()
-	m.xxx_TestEnum = o.TestEnum()
-	m.xxx_TestSigned = o.TestSigned()
-	m.SetTestFloat(o.TestFloat())
-	m.xxx_TestBoolEnum = o.TestBoolEnum()
-	m.SetTestScaledEnum(o.TestScaledEnum())
+func (m *IODebug) CopyFrom(o *IODebug) *IODebug {
+	_ = m.UnmarshalFrame(o.Frame())
 	return m
 }
 
@@ -992,8 +977,8 @@ type IOFloat32Reader interface {
 
 // IOFloat32Writer provides write access to a IOFloat32 message.
 type IOFloat32Writer interface {
-	// CopyFrom copies all values from IOFloat32Reader.
-	CopyFrom(IOFloat32Reader) *IOFloat32
+	// CopyFrom copies all values from IOFloat32.
+	CopyFrom(*IOFloat32) *IOFloat32
 	// SetFloat32ValueNoRange sets the value of the Float32ValueNoRange signal.
 	SetFloat32ValueNoRange(float32) *IOFloat32
 	// SetFloat32WithRange sets the physical value of the Float32WithRange signal.
@@ -1016,9 +1001,8 @@ func (m *IOFloat32) Reset() {
 	m.xxx_Float32WithRange = 0
 }
 
-func (m *IOFloat32) CopyFrom(o IOFloat32Reader) *IOFloat32 {
-	m.xxx_Float32ValueNoRange = o.Float32ValueNoRange()
-	m.SetFloat32WithRange(o.Float32WithRange())
+func (m *IOFloat32) CopyFrom(o *IOFloat32) *IOFloat32 {
+	_ = m.UnmarshalFrame(o.Frame())
 	return m
 }