From aef1e7df0f08701b04b82bfeff9ae24003da6e76 Mon Sep 17 00:00:00 2001 From: Jacob Jonsson Date: Thu, 16 May 2024 11:03:56 +0200 Subject: [PATCH] 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 | 27 ++++++++++ internal/generate/file.go | 14 ++--- testdata/gen/go/example/example.dbc.go | 72 ++++++++++---------------- 3 files changed, 59 insertions(+), 54 deletions(-) diff --git a/internal/generate/compile_test.go b/internal/generate/compile_test.go index 249e6a2..052815d 100644 --- a/internal/generate/compile_test.go +++ b/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()) + } +} diff --git a/internal/generate/file.go b/internal/generate/file.go index 3c1fb24..85a442a 100644 --- a/internal/generate/file.go +++ b/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() diff --git a/testdata/gen/go/example/example.dbc.go b/testdata/gen/go/example/example.dbc.go index 787167d..35d61fd 100644 --- a/testdata/gen/go/example/example.dbc.go +++ b/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 }