From 04cf4f562a6859ec25e0c8741ebca217a3c5bad2 Mon Sep 17 00:00:00 2001 From: erzoe Date: Tue, 9 Jan 2024 17:59:54 +0100 Subject: [PATCH 1/2] MessageId Standard vs Extended enum In dbc files the information whether a message is an extended frame or standard frame is encoded in bit 31 of the message ID. This is an implementation detail that I believe most users are not familiar with and that I have not seen in the documentation of this crate. Therefore I think it is very unintuitive to expose this to the users of this crate. Therefore I have replaced the previous MessageId struct which only contained the u32 as it was saved in the file with the MessageId enum proposed by schphil. https://github.com/marcelbuesing/can-dbc/pull/10#issuecomment-1599205250 The message id is now masked and can be directly compared to the message ids of incoming CAN bus messages. The information whether it's a standard frame or extended frame is intuitively available. This does not only make the usage of the crate more intuitive but is also helpful in case this crate ever wants to support more file formats such as sym files where this information is encoded differently. For more information on dbc file specifics see the DBC_File_Format_Documentation: http://mcu.so/Microcontroller/Automotive/dbc-file-format-documentation_compress.pdf This commit is, of course, a breaking change. But given that this crate is at version 5.0.0 it does not seem like it has a strict policy of avoiding breaking changes. If breaking changes are a concern we could rename the new enum to FrameId, restore the old struct and add both of them to the Message struct. That might confuse users, though, which field to use and would require good documentation. --- src/lib.rs | 29 ++++++++++++++++------------- src/parser.rs | 31 +++++++++++++++++++------------ 2 files changed, 35 insertions(+), 25 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index d1e0d74..035e827 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -153,7 +153,7 @@ SIG_VALTYPE_ 2000 Signal_8 : 1; fn lookup_signal_comment() { let dbc_content = DBC::try_from(SAMPLE_DBC).expect("Failed to parse DBC"); let comment = dbc_content - .signal_comment(MessageId(1840), "Signal_4") + .signal_comment(MessageId::Standard(1840), "Signal_4") .expect("Signal comment missing"); assert_eq!( "asaklfjlsdfjlsdfgls\nHH?=(%)/&KKDKFSDKFKDFKSDFKSDFNKCnvsdcvsvxkcv", @@ -164,7 +164,7 @@ SIG_VALTYPE_ 2000 Signal_8 : 1; #[test] fn lookup_signal_comment_none_when_missing() { let dbc_content = DBC::try_from(SAMPLE_DBC).expect("Failed to parse DBC"); - let comment = dbc_content.signal_comment(MessageId(1840), "Signal_2"); + let comment = dbc_content.signal_comment(MessageId::Standard(1840), "Signal_2"); assert_eq!(None, comment); } @@ -172,7 +172,7 @@ SIG_VALTYPE_ 2000 Signal_8 : 1; fn lookup_message_comment() { let dbc_content = DBC::try_from(SAMPLE_DBC).expect("Failed to parse DBC"); let comment = dbc_content - .message_comment(MessageId(1840)) + .message_comment(MessageId::Standard(1840)) .expect("Message comment missing"); assert_eq!("Some Message comment", comment); } @@ -180,7 +180,7 @@ SIG_VALTYPE_ 2000 Signal_8 : 1; #[test] fn lookup_message_comment_none_when_missing() { let dbc_content = DBC::try_from(SAMPLE_DBC).expect("Failed to parse DBC"); - let comment = dbc_content.message_comment(MessageId(2000)); + let comment = dbc_content.message_comment(MessageId::Standard(2000)); assert_eq!(None, comment); } @@ -188,7 +188,7 @@ SIG_VALTYPE_ 2000 Signal_8 : 1; fn lookup_value_descriptions_for_signal() { let dbc_content = DBC::try_from(SAMPLE_DBC).expect("Failed to parse DBC"); let val_descriptions = dbc_content - .value_descriptions_for_signal(MessageId(2000), "Signal_3") + .value_descriptions_for_signal(MessageId::Standard(2000), "Signal_3") .expect("Message comment missing"); let exp = vec![ValDescription { @@ -202,7 +202,7 @@ SIG_VALTYPE_ 2000 Signal_8 : 1; fn lookup_value_descriptions_for_signal_none_when_missing() { let dbc_content = DBC::try_from(SAMPLE_DBC).expect("Failed to parse DBC"); let val_descriptions = - dbc_content.value_descriptions_for_signal(MessageId(2000), "Signal_2"); + dbc_content.value_descriptions_for_signal(MessageId::Standard(2000), "Signal_2"); assert_eq!(None, val_descriptions); } @@ -210,7 +210,7 @@ SIG_VALTYPE_ 2000 Signal_8 : 1; fn lookup_extended_value_type_for_signal() { let dbc_content = DBC::try_from(SAMPLE_DBC).expect("Failed to parse DBC"); let extended_value_type = - dbc_content.extended_value_type_for_signal(MessageId(2000), "Signal_8"); + dbc_content.extended_value_type_for_signal(MessageId::Standard(2000), "Signal_8"); assert_eq!( extended_value_type, Some(&SignalExtendedValueType::IEEEfloat32Bit) @@ -221,28 +221,28 @@ SIG_VALTYPE_ 2000 Signal_8 : 1; fn lookup_extended_value_type_for_signal_none_when_missing() { let dbc_content = DBC::try_from(SAMPLE_DBC).expect("Failed to parse DBC"); let extended_value_type = - dbc_content.extended_value_type_for_signal(MessageId(2000), "Signal_1"); + dbc_content.extended_value_type_for_signal(MessageId::Standard(2000), "Signal_1"); assert_eq!(extended_value_type, None); } #[test] fn lookup_signal_by_name() { let dbc_content = DBC::try_from(SAMPLE_DBC).expect("Failed to parse DBC"); - let signal = dbc_content.signal_by_name(MessageId(2000), "Signal_8"); + let signal = dbc_content.signal_by_name(MessageId::Standard(2000), "Signal_8"); assert!(signal.is_some()); } #[test] fn lookup_signal_by_name_none_when_missing() { let dbc_content = DBC::try_from(SAMPLE_DBC).expect("Failed to parse DBC"); - let signal = dbc_content.signal_by_name(MessageId(2000), "Signal_25"); + let signal = dbc_content.signal_by_name(MessageId::Standard(2000), "Signal_25"); assert_eq!(signal, None); } #[test] fn lookup_multiplex_indicator_switch() { let dbc_content = DBC::try_from(SAMPLE_DBC).expect("Failed to parse DBC"); - let multiplexor_switch = dbc_content.message_multiplexor_switch(MessageId(3040)); + let multiplexor_switch = dbc_content.message_multiplexor_switch(MessageId::Standard(3040)); assert!(multiplexor_switch.is_ok()); assert!(multiplexor_switch.as_ref().unwrap().is_some()); assert_eq!(multiplexor_switch.unwrap().unwrap().name(), "Switch"); @@ -251,7 +251,7 @@ SIG_VALTYPE_ 2000 Signal_8 : 1; #[test] fn lookup_multiplex_indicator_switch_none_when_missing() { let dbc_content = DBC::try_from(SAMPLE_DBC).expect("Failed to parse DBC"); - let multiplexor_switch = dbc_content.message_multiplexor_switch(MessageId(1840)); + let multiplexor_switch = dbc_content.message_multiplexor_switch(MessageId::Standard(1840)); assert!(multiplexor_switch.unwrap().is_none()); } } @@ -298,7 +298,10 @@ pub struct Signal { /// Must be unique in DBC file. #[derive(Copy, Clone, Debug, PartialEq)] #[cfg_attr(feature = "serde", derive(Serialize))] -pub struct MessageId(pub u32); +pub enum MessageId { + Standard(u16), + Extended(u32), +} #[derive(Clone, Debug, PartialEq)] #[cfg_attr(feature = "serde", derive(Serialize))] diff --git a/src/parser.rs b/src/parser.rs index 9ce22fd..6c1af98 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -124,7 +124,7 @@ mod tests { #[test] fn signal_comment_test() { let def1 = "CM_ SG_ 193 KLU_R_X \"This is a signal comment test\";\n"; - let message_id = MessageId(193); + let message_id = MessageId::Standard(193); let comment1 = Comment::Signal { message_id, signal_name: "KLU_R_X".to_string(), @@ -137,7 +137,7 @@ mod tests { #[test] fn message_definition_comment_test() { let def1 = "CM_ BO_ 34544 \"Some Message comment\";\n"; - let message_id = MessageId(34544); + let message_id = MessageId::Standard(34544); let comment1 = Comment::Message { message_id, comment: "Some Message comment".to_string(), @@ -172,7 +172,7 @@ mod tests { #[test] fn value_description_for_signal_test() { let def1 = "VAL_ 837 UF_HZ_OI 255 \"NOP\";\n"; - let message_id = MessageId(837); + let message_id = MessageId::Standard(837); let signal_name = "UF_HZ_OI".to_string(); let val_descriptions = vec![ValDescription { a: 255.0, @@ -243,7 +243,7 @@ mod tests { fn message_definition_attribute_value_test() { let def = "BA_ \"AttrName\" BO_ 298 13;\n"; let attribute_value = AttributeValuedForObjectType::MessageDefinitionAttributeValue( - MessageId(298), + MessageId::Standard(298), Some(AttributeValue::AttributeValueF64(13.0)), ); let attr_val_exp = AttributeValueForObject { @@ -258,7 +258,7 @@ mod tests { fn signal_attribute_value_test() { let def = "BA_ \"AttrName\" SG_ 198 SGName 13;\n"; let attribute_value = AttributeValuedForObjectType::SignalAttributeValue( - MessageId(198), + MessageId::Standard(198), "SGName".to_string(), AttributeValue::AttributeValueF64(13.0), ); @@ -377,7 +377,7 @@ mod tests { let def = "SIG_GROUP_ 23 X_3290 1 : A_b XY_Z;\n"; let exp = SignalGroups { - message_id: MessageId(23), + message_id: MessageId::Standard(23), signal_group_name: "X_3290".to_string(), repetitions: 1, signal_names: vec!["A_b".to_string(), "XY_Z".to_string()], @@ -442,7 +442,7 @@ mod tests { fn message_transmitters_test() { let def = "BO_TX_BU_ 12345 : XZY,ABC;\n"; let exp = MessageTransmitter { - message_id: MessageId(12345), + message_id: MessageId::Standard(12345), transmitter: vec![ Transmitter::NodeName("XZY".to_string()), Transmitter::NodeName("ABC".to_string()), @@ -502,7 +502,7 @@ mod tests { // simple mapping let def = "SG_MUL_VAL_ 2147483650 muxed_A_1 MUX_A 1-1;\n"; let exp = ExtendedMultiplex { - message_id: MessageId(2147483650), + message_id: MessageId::Extended(2), signal_name: "muxed_A_1".to_string(), multiplexor_signal_name: "MUX_A".to_string(), mappings: vec![ExtendedMultiplexMapping { @@ -516,7 +516,7 @@ mod tests { // range mapping let def = "SG_MUL_VAL_ 2147483650 muxed_A_1 MUX_A 1568-2568;\n"; let exp = ExtendedMultiplex { - message_id: MessageId(2147483650), + message_id: MessageId::Extended(2), signal_name: "muxed_A_1".to_string(), multiplexor_signal_name: "MUX_A".to_string(), mappings: vec![ExtendedMultiplexMapping { @@ -530,7 +530,7 @@ mod tests { // multiple mappings let def = "SG_MUL_VAL_ 2147483650 muxed_B_5 MUX_B 5-5, 16-24;\n"; let exp = ExtendedMultiplex { - message_id: MessageId(2147483650), + message_id: MessageId::Extended(2), signal_name: "muxed_B_5".to_string(), multiplexor_signal_name: "MUX_B".to_string(), mappings: vec![ @@ -552,7 +552,7 @@ mod tests { fn sig_val_type_test() { let def = "SIG_VALTYPE_ 2000 Signal_8 : 1;\n"; let exp = SignalExtendedValueTypeList { - message_id: MessageId(2000), + message_id: MessageId::Standard(2000), signal_name: "Signal_8".to_string(), signal_extended_value_type: SignalExtendedValueType::IEEEfloat32Bit, }; @@ -687,7 +687,14 @@ fn byte_order(s: &str) -> IResult<&str, ByteOrder> { } fn message_id(s: &str) -> IResult<&str, MessageId> { - map(complete::u32, MessageId)(s) + let (s, parsed_value) = complete::u32(s)?; + + if parsed_value > u16::MAX as u32 { + let extended_value = parsed_value & u32::from(u16::MAX); + Ok((s, MessageId::Extended(extended_value & 0x1FFFFFFF))) + } else { + Ok((s, MessageId::Standard(parsed_value as u16))) + } } fn signed(s: &str) -> IResult<&str, ValueType> { From 5d20fa8c0ab10c703f1e99ceee89174e2c2d290e Mon Sep 17 00:00:00 2001 From: erzoe Date: Wed, 7 Feb 2024 07:52:15 +0100 Subject: [PATCH 2/2] changed check extended id vs standard id as requested by https://github.com/marcelbuesing/can-dbc/pull/13#pullrequestreview-1866333726 --- src/parser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/parser.rs b/src/parser.rs index 6c1af98..e440d34 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -689,7 +689,7 @@ fn byte_order(s: &str) -> IResult<&str, ByteOrder> { fn message_id(s: &str) -> IResult<&str, MessageId> { let (s, parsed_value) = complete::u32(s)?; - if parsed_value > u16::MAX as u32 { + if parsed_value & (1 << 31) != 0 { let extended_value = parsed_value & u32::from(u16::MAX); Ok((s, MessageId::Extended(extended_value & 0x1FFFFFFF))) } else {