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.

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

IGNORE_OVERLAP in telemetry #1208

Merged
merged 3 commits into from
Apr 29, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
7 changes: 6 additions & 1 deletion openc3/data/config/telemetry_modifiers.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -170,4 +170,9 @@ ACCESSOR:
required: true
description: The name of the accessor class
values: .+
since: 5.0.10
since: 5.0.10
IGNORE_OVERLAP:
summary: Ignores any packet items which overlap
description: Packet items which overlap normally generate a warning unless each individual item has the OVERLAP keyword.
This ignores overlaps across the entire packet.
since: 5.15.3
51 changes: 27 additions & 24 deletions openc3/lib/openc3/packets/packet.rb
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,9 @@ class Packet < Structure
# @return [Array<Array<Target Name, Packet Name, Item Name>>] Related items
attr_accessor :related_items

# @return [Boolean] Whether to ignore overlapping items
attr_accessor :ignore_overlap

# Valid format types
VALUE_TYPES = [:RAW, :CONVERTED, :FORMATTED, :WITH_UNITS]

Expand Down Expand Up @@ -140,6 +143,7 @@ def initialize(target_name = nil, packet_name = nil, default_endianness = :BIG_E
@cmd_or_tlm = nil
@template = nil
@packet_time = nil
@ignore_overlap = false
end

# Sets the target name this packet is associated with. Unidentified packets
Expand All @@ -151,12 +155,10 @@ def target_name=(target_name)
if !(String === target_name)
raise(ArgumentError, "target_name must be a String but is a #{target_name.class}")
end

@target_name = target_name.upcase.freeze
else
@target_name = nil
end
@target_name
Copy link
Member

Choose a reason for hiding this comment

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

I understand this is redundant, but I like explicit returns. Should be "return @target_name"

Copy link
Member Author

Choose a reason for hiding this comment

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

A setter should not have a return. That's why I removed it ... not because it was implicit

Copy link
Member Author

Choose a reason for hiding this comment

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

end

# Sets the packet name. Unidentified packets will have packet name set to
Expand All @@ -168,12 +170,10 @@ def packet_name=(packet_name)
if !(String === packet_name)
raise(ArgumentError, "packet_name must be a String but is a #{packet_name.class}")
end

@packet_name = packet_name.upcase.freeze
else
@packet_name = nil
end
@packet_name
end

# Sets the description of the packet
Expand All @@ -184,12 +184,10 @@ def description=(description)
if !(String === description)
raise(ArgumentError, "description must be a String but is a #{description.class}")
end

@description = description.to_utf8.freeze
else
@description = nil
end
@description
end

# Sets the received time of the packet
Expand All @@ -200,13 +198,11 @@ def received_time=(received_time)
if !(Time === received_time)
raise(ArgumentError, "received_time must be a Time but is a #{received_time.class}")
end

@received_time = received_time.clone.freeze
else
@received_time = nil
end
@read_conversion_cache.clear if @read_conversion_cache
@received_time
end

# Sets the received count of the packet
Expand All @@ -217,10 +213,8 @@ def received_count=(received_count)
if !(Integer === received_count)
raise(ArgumentError, "received_count must be an Integer but is a #{received_count.class}")
end

@received_count = received_count
@read_conversion_cache.clear if @read_conversion_cache
@received_count
end

end # if RUBY_ENGINE != 'ruby' or ENV['OPENC3_NO_EXT']
Expand Down Expand Up @@ -401,6 +395,10 @@ def template=(template)
#
# @return [Array<String>] Warning messages for big definition overlaps
def check_bit_offsets
if @ignore_overlap
Logger.instance.info("#{@target_name} #{@packet_name} has IGNORE_OVERLAP so bit overlaps ignored")
return []
end
expected_next_offset = nil
previous_item = nil
warnings = []
Expand Down Expand Up @@ -617,7 +615,7 @@ def read_item(item, value_type = :CONVERTED, buffer = @buffer, given_raw = nil)

unless using_cached_value
if item.array_size
value.map! do |val, index|
value.map! do |val, _index|
item.read_conversion.call(val, self, buffer)
end
else
Expand Down Expand Up @@ -646,7 +644,7 @@ def read_item(item, value_type = :CONVERTED, buffer = @buffer, given_raw = nil)
# Convert from value to state if possible
if item.states
if Array === value
value = value.map do |val, index|
value = value.map do |val, _index|
if item.states.key(val)
item.states.key(val)
elsif item.states.values.include?(ANY_STATE)
Expand All @@ -667,7 +665,7 @@ def read_item(item, value_type = :CONVERTED, buffer = @buffer, given_raw = nil)
end
else
if Array === value
value = value.map do |val, index|
value = value.map do |val, _index|
apply_format_string_and_units(item, val, value_type)
end
else
Expand All @@ -693,7 +691,7 @@ def read_item(item, value_type = :CONVERTED, buffer = @buffer, given_raw = nil)
# @param value_type [Symbol] Value type to read for every item
# @param buffer [String] The binary buffer to read the items from
# @return Hash of read names and values
def read_items(items, value_type = :RAW, buffer = @buffer, raw_value = nil)
def read_items(items, value_type = :RAW, buffer = @buffer, _raw_value = nil)
buffer = allocate_buffer_if_needed() unless buffer
if value_type == :RAW
result = super(items, value_type, buffer)
Expand Down Expand Up @@ -735,11 +733,11 @@ def write_item(item, value, value_type = :CONVERTED, buffer = @buffer)
end
begin
super(item, value, :RAW, buffer)
rescue ArgumentError => err
if item.states and String === value and err.message =~ /invalid value for/
rescue ArgumentError => e
if item.states and String === value and e.message =~ /invalid value for/
raise "Unknown state #{value} for #{item.name}"
else
raise err
raise e
end
end
when :FORMATTED, :WITH_UNITS
Expand Down Expand Up @@ -998,7 +996,7 @@ def reset
end
return unless @processors

@processors.each do |processor_name, processor|
@processors.each do |_processor_name, processor|
processor.reset
end
end
Expand Down Expand Up @@ -1051,7 +1049,7 @@ def to_config(cmd_or_tlm)
config << "COMMAND #{@target_name.to_s.quote_if_necessary} #{@packet_name.to_s.quote_if_necessary} #{@default_endianness} \"#{@description}\"\n"
end
if @accessor.class.to_s != 'OpenC3::BinaryAccessor'
config << " ACCESSOR #{@accessor.class.to_s} #{@accessor.args.map { |a| a.to_s.quote_if_necessary }.join(" ")}\n"
config << " ACCESSOR #{@accessor.class} #{@accessor.args.map { |a| a.to_s.quote_if_necessary }.join(" ")}\n"
end
# TODO: Add TEMPLATE_ENCODED so this can always be done inline regardless of content
if @template
Expand All @@ -1067,7 +1065,7 @@ def to_config(cmd_or_tlm)
end

if @processors
@processors.each do |processor_name, processor|
@processors.each do |_processor_name, processor|
config << processor.to_config
end
end
Expand Down Expand Up @@ -1106,6 +1104,9 @@ def to_config(cmd_or_tlm)
config << " RELATED_ITEM #{target_name.to_s.quote_if_necessary} #{packet_name.to_s.quote_if_necessary} #{item_name.to_s.quote_if_necessary}"
end
end
if @ignore_overlap
config << " IGNORE_OVERLAP"
end
config
end

Expand All @@ -1128,7 +1129,7 @@ def as_json(*a)

if @processors
processors = []
@processors.each do |processor_name, processor|
@processors.each do |_processor_name, processor|
processors << processor.as_json(*a)
end
config['processors'] = processors
Expand Down Expand Up @@ -1161,6 +1162,7 @@ def as_json(*a)
if @related_items
config['related_items'] = @related_items
end
config['ignore_overlap'] = true if @ignore_overlap

config
end
Expand All @@ -1182,8 +1184,8 @@ def self.from_json(hash)
else
packet.accessor = accessor.new(packet)
end
rescue => error
Logger.instance.error "#{packet.target_name} #{packet.packet_name} accessor of #{hash['accessor']} could not be found due to #{error}"
rescue => e
Logger.instance.error "#{packet.target_name} #{packet.packet_name} accessor of #{hash['accessor']} could not be found due to #{e}"
end
end
packet.template = Base64.decode64(hash['template']) if hash['template']
Expand All @@ -1204,6 +1206,7 @@ def self.from_json(hash)
if hash['related_items']
packet.related_items = hash['related_items']
end
packet.ignore_overlap = hash['ignore_overlap']

packet
end
Expand Down Expand Up @@ -1237,7 +1240,7 @@ def decom
def process(buffer = @buffer)
return unless @processors

@processors.each do |processor_name, processor|
@processors.each do |_processor_name, processor|
processor.call(self, buffer)
end
end
Expand Down
7 changes: 6 additions & 1 deletion openc3/lib/openc3/packets/packet_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ def process_file(filename, process_target_name, language = 'ruby')
'APPEND_PARAMETER', 'APPEND_ID_ITEM', 'APPEND_ID_PARAMETER', 'APPEND_ARRAY_ITEM',\
'APPEND_ARRAY_PARAMETER', 'ALLOW_SHORT', 'HAZARDOUS', 'PROCESSOR', 'META',\
'DISABLE_MESSAGES', 'HIDDEN', 'DISABLED', 'ACCESSOR', 'TEMPLATE', 'TEMPLATE_FILE',\
'RESPONSE', 'ERROR_RESPONSE', 'SCREEN', 'RELATED_ITEM'
'RESPONSE', 'ERROR_RESPONSE', 'SCREEN', 'RELATED_ITEM', 'IGNORE_OVERLAP'
raise parser.error("No current packet for #{keyword}") unless @current_packet

process_current_packet(parser, keyword, params)
Expand Down Expand Up @@ -539,6 +539,11 @@ def process_current_packet(parser, keyword, params)
@current_packet.related_items ||= []
@current_packet.related_items << [params[0].upcase, params[1].upcase, params[2].upcase]

when 'IGNORE_OVERLAP'
usage = "#{keyword}"
parser.verify_num_parameters(0, 0, usage)
@current_packet.ignore_overlap = true

end

end
Expand Down
22 changes: 15 additions & 7 deletions openc3/python/openc3/packets/packet.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ def __init__(
self.error_response = None
self.screen = None
self.related_items = None
self.ignore_overlap = False

@property
def target_name(self):
Expand Down Expand Up @@ -345,8 +346,14 @@ def template(self, template):
# gaps in the packet, but not allow the same bits to be used for multiple
# variables.
#
# self.return [Array<String>] Warning messages for big definition overlaps
# self.return [Array<String>] Warning messages for bit definition overlaps
def check_bit_offsets(self):
if self.ignore_overlap:
Logger.info(
f"{self.target_name} {self.packet_name} has IGNORE_OVERLAP so bit overlaps ignored"
)
return []

expected_next_offset = None
previous_item = None
warnings = []
Expand Down Expand Up @@ -1042,6 +1049,9 @@ def to_config(self, cmd_or_tlm):
for related_item in self.related_items:
config += f" RELATED_ITEM {quote_if_necessary(related_item[0])} {quote_if_necessary(related_item[1])} {quote_if_necessary(related_item[2])}\n"

if self.ignore_overlap:
config += " IGNORE_OVERLAP"

return config

def as_json(self):
Expand Down Expand Up @@ -1089,15 +1099,14 @@ def as_json(self):

if self.response:
config["response"] = self.response

if self.error_response:
config["error_response"] = self.error_response

if self.screen:
config["screen"] = self.screen

if self.related_items:
config["related_items"] = self.related_items
if self.ignore_overlap:
config["ignore_overlap"] = self.ignore_overlap

return config

Expand Down Expand Up @@ -1136,15 +1145,14 @@ def from_json(cls, hash):

if "response" in hash:
packet.response = hash["response"]

if "error_response" in hash:
packet.error_response = hash["error_response"]

if "screen" in hash:
packet.screen = hash["screen"]

if "related_items" in hash:
packet.related_items = hash["related_items"]
if "ignore_overlap" in hash:
packet.ignore_overlap = hash["ignore_overlap"]

return packet

Expand Down
6 changes: 6 additions & 0 deletions openc3/python/openc3/packets/packet_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ def process_file(self, filename, process_target_name):
| "ERROR_RESPONSE"
| "SCREEN"
| "RELATED_ITEM"
| "IGNORE_OVERLAP"
):
if not self.current_packet:
raise parser.error(f"No current packet for {keyword}")
Expand Down Expand Up @@ -533,6 +534,11 @@ def process_current_packet(self, parser, keyword, params):
[params[0].upper(), params[1].upper(), params[2].upper()]
)

case "IGNORE_OVERLAP":
usage = f"{keyword}"
parser.verify_num_parameters(0, 0, usage)
self.current_packet.ignore_overlap = True

def process_current_item(self, parser, keyword, params):
match keyword:
# Add a state to the current telemety item
Expand Down
24 changes: 24 additions & 0 deletions openc3/python/test/packets/test_packet_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -1229,3 +1229,27 @@ def test_allows_appending_derived_items(self):
self.pc.telemetry["TGT1"]["PKT1"].items["ITEM1"].data_type, "DERIVED"
)
tf.close()

def test_detects_overlapping_items_without_IGNORE_OVERLAP(self):
tf = tempfile.NamedTemporaryFile(mode="w")
tf.write('TELEMETRY tgt1 pkt1 LITTLE_ENDIAN "Packet"\n')
tf.write(" ITEM item1 0 8 UINT\n")
tf.write(" ITEM item2 4 4 UINT\n")
tf.seek(0)
self.pc.process_file(tf.name, "TGT1")
self.assertIn(
"Bit definition overlap at bit offset 4 for packet TGT1 PKT1 items ITEM2 and ITEM1",
self.pc.warnings,
)
tf.close()

def test_ignores_overlapping_items_with_IGNORE_OVERLAP(self):
tf = tempfile.NamedTemporaryFile(mode="w")
tf.write('TELEMETRY tgt1 pkt1 LITTLE_ENDIAN "Packet"\n')
tf.write(" IGNORE_OVERLAP\n")
tf.write(" ITEM item1 0 8 UINT\n")
tf.write(" ITEM item2 4 4 UINT\n")
tf.seek(0)
self.pc.process_file(tf.name, "TGT1")
self.assertEqual(len(self.pc.warnings), 0)
tf.close()
Loading
Loading