Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly parse hex and binary ID items #1896

Merged
merged 2 commits into from
Feb 13, 2025
Merged

Properly parse hex and binary ID items #1896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fdd731c
Properly parse hex and binary ID items
jmthomas Feb 12, 2025
ad188ad
Update copywright
jmthomas Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 28 additions & 21 deletions openc3/lib/openc3/packets/parsers/packet_item_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
# GNU Affero General Public License for more details.

# Modified by OpenC3, Inc.
# All changes Copyright 2023, OpenC3, Inc.
# All changes Copyright 2025, OpenC3, Inc.
# All Rights Reserved
#
# This file may also be used under the terms of a commercial license
Expand Down Expand Up @@ -80,7 +80,7 @@ def create_packet_item(packet, cmd_or_tlm)
item.range = get_range()
item.default = get_default()
end
item.id_value = get_id_value()
item.id_value = get_id_value(item)
item.description = get_description()
if append?
item = packet.append(item)
Expand Down Expand Up @@ -165,6 +165,18 @@ def get_range
min..max
end

def convert_string_value(index)
# If the default value is 0x<data> (no quotes), it is treated as
# binary data. Otherwise, the default value is considered to be a string.
if @parser.parameters[index].upcase.start_with?("0X") and
[email protected]?("\"#{@parser.parameters[index]}\"") and
[email protected]?("\'#{@parser.parameters[index]}\'")
return @parser.parameters[index].hex_to_byte_string
else
return @parser.parameters[index]
end
end

def get_default
return [] if @parser.keyword.include?('ARRAY')

Expand All @@ -173,15 +185,7 @@ def get_default
return [] if data_type == :ARRAY
return {} if data_type == :OBJECT
if data_type == :STRING or data_type == :BLOCK
# If the default value is 0x<data> (no quotes), it is treated as
# binary data. Otherwise, the default value is considered to be a string.
if @parser.parameters[index].upcase.start_with?("0X") and
[email protected]?("\"#{@parser.parameters[index]}\"") and
[email protected]?("\'#{@parser.parameters[index]}\'")
return @parser.parameters[index].hex_to_byte_string
else
return @parser.parameters[index]
end
return convert_string_value(index)
else
if data_type != :DERIVED
return ConfigParser.handle_defined_constants(
Expand All @@ -193,22 +197,25 @@ def get_default
end
end

def get_id_value
def get_id_value(item)
return nil unless @parser.keyword.include?('ID_')

data_type = get_data_type
if @parser.keyword.include?('ITEM')
index = append? ? 3 : 4
else # PARAMETER
index = append? ? 5 : 6
# STRING and BLOCK PARAMETERS don't have min and max values
index -= 2 if data_type == :STRING || data_type == :BLOCK
end
if data_type == :DERIVED
raise @parser.error("DERIVED data type not allowed for Identifier")
end
# For PARAMETERS the default value is the ID value
if @parser.keyword.include?("PARAMETER")
return item.default
end
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to reparse the ID value in commands ... just use the default


@parser.parameters[index]
index = append? ? 3 : 4
if data_type == :STRING or data_type == :BLOCK
return convert_string_value(index)
else
return ConfigParser.handle_defined_constants(
@parser.parameters[index].convert_to_value, data_type, get_bit_size()
)
end
end

def get_description
Expand Down
1 change: 1 addition & 0 deletions openc3/openc3.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ spec = Gem::Specification.new do |s|
s.add_runtime_dependency 'opentelemetry-instrumentation-rack', '~> 0.21'
s.add_runtime_dependency 'opentelemetry-instrumentation-redis', '~> 0.24'
s.add_runtime_dependency 'opentelemetry-sdk', '~> 1.2'
s.add_runtime_dependency 'resolv-replace', '~> 0.1.1'
s.add_runtime_dependency 'rufus-scheduler', '~> 3.8'
s.add_runtime_dependency 'tzinfo-data', '~> 1.2023'
s.add_runtime_dependency 'webrick', '~> 1.8'
Expand Down
51 changes: 29 additions & 22 deletions openc3/python/openc3/packets/parsers/packet_item_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright 2023 OpenC3, Inc.
# Copyright 2025 OpenC3, Inc.
# All Rights Reserved.
#
# This program is free software; you can modify and/or redistribute it
Expand Down Expand Up @@ -80,7 +80,7 @@ def create_packet_item(self, packet, cmd_or_tlm):
item.minimum = self._get_minimum()
item.maximum = self._get_maximum()
item.default = self._get_default()
item.id_value = self._get_id_value()
item.id_value = self._get_id_value(item)
item.description = self._get_description()
if self._append():
item = packet.append(item)
Expand Down Expand Up @@ -185,23 +185,26 @@ def _get_maximum(self):
self._get_bit_size(),
)

def _convert_string_value(self, index):
# If the default value is 0x<data> (no quotes), it is treated as
# binary data. Otherwise, the default value is considered to be a string.
if (
self.parser.parameters[index].upper().startswith("0X")
and f'"{self.parser.parameters[index]}"' not in self.parser.line
and f"'{self.parser.parameters[index]}'" not in self.parser.line
):
return hex_to_byte_string(self.parser.parameters[index])
else:
return self.parser.parameters[index]

def _get_default(self):
if "ARRAY" in self.parser.keyword:
return []

index = 3 if self._append() else 4
data_type = self._get_data_type()
if data_type == "STRING" or data_type == "BLOCK":
# If the default value is 0x<data> (no quotes), it is treated as
# binary data. Otherwise, the default value is considered to be a string.
if (
self.parser.parameters[index].upper().startswith("0X")
and f'"{self.parser.parameters[index]}"' not in self.parser.line
and f"'{self.parser.parameters[index]}'" not in self.parser.line
):
return hex_to_byte_string(self.parser.parameters[index])
else:
return self.parser.parameters[index]
return self._convert_string_value(index)
else:
if data_type != "DERIVED":
return ConfigParser.handle_defined_constants(
Expand All @@ -212,21 +215,25 @@ def _get_default(self):
else:
return convert_to_value(self.parser.parameters[index + 2])

def _get_id_value(self):
def _get_id_value(self, item):
if "ID_" not in self.parser.keyword:
return None

data_type = self._get_data_type()
if "ITEM" in self.parser.keyword:
index = 3 if self._append() else 4
else: # PARAMETER
index = 5 if self._append() else 6
# STRING and BLOCK PARAMETERS don't have min and max values
if data_type == "STRING" or data_type == "BLOCK":
index -= 2
if data_type == "DERIVED":
raise self.parser.error("DERIVED data type not allowed for Identifier")
return self.parser.parameters[index]
# For PARAMETERS the default value is the ID value
if "PARAMETER" in self.parser.keyword:
return item.default

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to reparse the ID value in commands ... just use the default

index = 3 if self._append() else 4
if data_type == "STRING" or data_type == "BLOCK":
return self._convert_string_value(index)
else:
return ConfigParser.handle_defined_constants(
convert_to_value(self.parser.parameters[index]),
self._get_data_type(),
self._get_bit_size(),
)

def _get_description(self):
max_options = self.usage.count("<")
Expand Down
32 changes: 21 additions & 11 deletions openc3/python/test/packets/parsers/test_packet_item_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright 2023 OpenC3, Inc.
# Copyright 2025 OpenC3, Inc.
# All Rights Reserved.
#
# This program is free software; you can modify and/or redistribute it
Expand Down Expand Up @@ -129,22 +129,27 @@ def test_only_allows_derived_items_with_offset_0_and_size_0(self):
def test_accepts_types_int_uint_float_string_block(self):
tf = tempfile.NamedTemporaryFile(mode="w")
tf.write('TELEMETRY tgt1 pkt1 LITTLE_ENDIAN "Description"\n')
tf.write(" ID_ITEM ITEM1 0 32 INT 0\n")
tf.write(" ID_ITEM ITEM1 0 32 INT 0x42\n")
tf.write(" ITEM ITEM2 0 32 UINT\n")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case currently fails in Python (but not Ruby) because Ruby calls Integer(value) which works ... the Python conversion code did not handle it.

tf.write(" ARRAY_ITEM ITEM3 0 32 FLOAT 64\n")
tf.write(' APPEND_ID_ITEM ITEM4 32 STRING "ABCD"\n')
tf.write(" APPEND_ITEM ITEM5 32 BLOCK\n")
tf.write(" APPEND_ARRAY_ITEM ITEM6 32 BLOCK 64\n")
tf.write(' APPEND_ID_ITEM ITEM4 32 STRING "0xABCD"\n')
tf.write(' APPEND_ID_ITEM ITEM5 32 BLOCK 0xABCD\n')
tf.write(" APPEND_ITEM ITEM6 32 BLOCK\n")
tf.write(" APPEND_ARRAY_ITEM ITEM7 32 BLOCK 64\n")
tf.seek(0)
self.pc.process_file(tf.name, "TGT1")
self.assertTrue(
set(["ITEM1", "ITEM2", "ITEM3", "ITEM4", "ITEM5", "ITEM6"]).issubset(
set(["ITEM1", "ITEM2", "ITEM3", "ITEM4", "ITEM5", "ITEM6", "ITEM7"]).issubset(
set(self.pc.telemetry["TGT1"]["PKT1"].items.keys())
),
)
self.assertEqual(self.pc.telemetry["TGT1"]["PKT1"].items["ITEM1"].id_value, 0x42)
self.assertEqual(self.pc.telemetry["TGT1"]["PKT1"].items["ITEM4"].id_value, "0xABCD")
self.assertEqual(self.pc.telemetry["TGT1"]["PKT1"].items["ITEM5"].id_value, b"\xAB\xCD")
id_items = []
id_items.append(self.pc.telemetry["TGT1"]["PKT1"].items["ITEM1"])
id_items.append(self.pc.telemetry["TGT1"]["PKT1"].items["ITEM4"])
id_items.append(self.pc.telemetry["TGT1"]["PKT1"].items["ITEM5"])
self.assertEqual(self.pc.telemetry["TGT1"]["PKT1"].id_items, id_items)
tf.close()

Expand Down Expand Up @@ -297,17 +302,22 @@ def test_doesnt_allow_append_id_parameter_with_derived_type(self):
def test_accepts_types_int_uint_float_string_block(self):
tf = tempfile.NamedTemporaryFile(mode="w")
tf.write('COMMAND tgt1 pkt1 LITTLE_ENDIAN "Description"\n')
tf.write(" ID_PARAMETER ITEM1 0 32 INT 0 0 0\n")
tf.write(' ID_PARAMETER ITEM2 32 32 STRING "ABCD"\n')
tf.write(" ID_PARAMETER ITEM1 0 32 INT MIN MAX 0x42\n")
tf.write(' ID_PARAMETER ITEM2 32 32 STRING "0xABCD"\n')
tf.write(" PARAMETER ITEM3 64 32 UINT 0 0 0\n")
tf.write(" ARRAY_PARAMETER ITEM4 96 32 FLOAT 64\n")
tf.write(" APPEND_ID_PARAMETER ITEM5 32 UINT 0 0 0\n")
tf.write(' APPEND_ID_PARAMETER ITEM6 32 STRING "ABCD"\n')
tf.write(" APPEND_ID_PARAMETER ITEM5 32 UINT 0x0 0xFFFFFFFF 0xDEADBEEF\n")
tf.write(' APPEND_ID_PARAMETER ITEM6 32 BLOCK 0xABCD\n')
tf.write(' APPEND_PARAMETER ITEM7 32 BLOCK "1234"\n')
tf.write(" APPEND_ARRAY_PARAMETER ITEM8 32 BLOCK 64\n")
tf.seek(0)
self.pc.process_file(tf.name, "TGT1")
self.assertIn("ITEM1", self.pc.commands["TGT1"]["PKT1"].items.keys())
packet = self.pc.commands["TGT1"]["PKT1"]
self.assertIn("ITEM1", packet.items.keys())
self.assertEqual(packet.get_item("ITEM1").id_value, 0x42)
self.assertEqual(packet.get_item("ITEM2").id_value, '0xABCD')
self.assertEqual(packet.get_item("ITEM5").id_value, 0xDEADBEEF)
self.assertEqual(packet.get_item("ITEM6").id_value, b'\xAB\xCD')
tf.close()

def test_supports_arbitrary_range_default_and_endianness_per_item(self):
Expand Down
19 changes: 12 additions & 7 deletions openc3/spec/packets/parsers/packet_item_parser_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
# GNU Affero General Public License for more details.

# Modified by OpenC3, Inc.
# All changes Copyright 2022, OpenC3, Inc.
# All changes Copyright 2025, OpenC3, Inc.
# All Rights Reserved
#
# This file may also be used under the terms of a commercial license
Expand Down Expand Up @@ -127,18 +127,23 @@ module OpenC3
it "accepts types INT UINT FLOAT STRING BLOCK" do
tf = Tempfile.new('unittest')
tf.puts 'TELEMETRY tgt1 pkt1 LITTLE_ENDIAN "Description"'
tf.puts ' ID_ITEM ITEM1 0 32 INT 0'
tf.puts ' ID_ITEM ITEM1 0 32 INT 0x42'
tf.puts ' ITEM ITEM2 0 32 UINT'
tf.puts ' ARRAY_ITEM ITEM3 0 32 FLOAT 64'
tf.puts ' APPEND_ID_ITEM ITEM4 32 STRING "ABCD"'
tf.puts ' APPEND_ITEM ITEM5 32 BLOCK'
tf.puts ' APPEND_ARRAY_ITEM ITEM6 32 BLOCK 64'
tf.puts ' APPEND_ID_ITEM ITEM4 32 STRING "0xABCD"'
tf.puts ' APPEND_ID_ITEM ITEM5 32 BLOCK 0xABCD'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ruby didn't support block ID items as binary

tf.puts ' APPEND_ITEM ITEM6 32 BLOCK'
tf.puts ' APPEND_ARRAY_ITEM ITEM7 32 BLOCK 64'
tf.close
@pc.process_file(tf.path, "TGT1")
expect(@pc.telemetry["TGT1"]["PKT1"].items.keys).to include('ITEM1', 'ITEM2', 'ITEM3', 'ITEM4', 'ITEM5', 'ITEM6')
expect(@pc.telemetry["TGT1"]["PKT1"].items.keys).to include('ITEM1', 'ITEM2', 'ITEM3', 'ITEM4', 'ITEM5', 'ITEM6', 'ITEM7')
expect(@pc.telemetry["TGT1"]["PKT1"].items["ITEM1"].id_value).to eql 0x42
expect(@pc.telemetry["TGT1"]["PKT1"].items["ITEM4"].id_value).to eql '0xABCD'
expect(@pc.telemetry["TGT1"]["PKT1"].items["ITEM5"].id_value).to eql "\xAB\xCD"
id_items = []
id_items << @pc.telemetry["TGT1"]["PKT1"].items["ITEM1"]
id_items << @pc.telemetry["TGT1"]["PKT1"].items["ITEM4"]
id_items << @pc.telemetry["TGT1"]["PKT1"].items["ITEM5"]
expect(@pc.telemetry["TGT1"]["PKT1"].id_items).to eql id_items
tf.unlink
end
Expand Down Expand Up @@ -293,7 +298,7 @@ module OpenC3
it "accepts types INT UINT FLOAT STRING BLOCK" do
tf = Tempfile.new('unittest')
tf.puts 'COMMAND tgt1 pkt1 LITTLE_ENDIAN "Description"'
tf.puts ' ID_PARAMETER ITEM1 0 32 INT 0 0 0'
tf.puts ' ID_PARAMETER ITEM1 0 32 INT 0 0xFF 0x42'
tf.puts ' ID_PARAMETER ITEM2 32 32 STRING "ABCD"'
tf.puts ' PARAMETER ITEM3 64 32 UINT 0 0 0'
tf.puts ' ARRAY_PARAMETER ITEM4 96 32 FLOAT 64'
Expand Down
Loading