Merge pull request #1866 from OpenC3/inject

Check for invalid inject_tlm states, rescue bad values

openc3/lib/openc3/api/tlm_api.rb

@@ -131,7 +131,16 @@ def inject_tlm(target_name, packet_name, item_hash = nil, type: :CONVERTED, manu
       if item_hash
         item_hash = item_hash.transform_keys(&:upcase)
         # Check that the items exist ... exceptions are raised if not
-        TargetModel.packet_items(target_name, packet_name, item_hash.keys, scope: scope)
+        items = TargetModel.packet_items(target_name, packet_name, item_hash.keys, scope: scope)
+        if type == :CONVERTED
+          # If the type is converted, check that the item states are valid
+          item_hash.each do |item_name, item_value|
+            item = items.find { |i| i['name'] == item_name.to_s.upcase }
+            if item['states'] && !item['states'][item_value]
+              raise "Unknown state '#{item_value}' for #{item['name']}, must be one of #{item['states'].keys.join(', ')}"
+            end
+          end
+        end
       else
         # Check that the packet exists ... exceptions are raised if not
         TargetModel.packet(target_name, packet_name, scope: scope)

openc3/lib/openc3/microservices/interface_decom_common.rb

@@ -36,6 +36,10 @@ def handle_inject_tlm(inject_tlm_json)
       packet.received_count += 1
       packet.received_time = Time.now.sys
       TelemetryTopic.write_packet(packet, scope: @scope)
+    # If the inject_tlm parameters are bad we rescue so
+    # interface_microservice and decom_microservice can continue
+    rescue => e
+      @logger.error "inject_tlm error due to #{e.message}"
     end
 
     def handle_build_cmd(build_cmd_json, msg_id)

openc3/lib/openc3/packets/packet.rb

@@ -756,7 +756,7 @@ def write_item(item, value, value_type = :CONVERTED, buffer = @buffer)
           super(item, value, :RAW, buffer)
         rescue ArgumentError => e
           if item.states and String === value and e.message =~ /invalid value for/
-            raise "Unknown state #{value} for #{item.name}, must be one of #{item.states.keys.join(', ')}"
+            raise "Unknown state '#{value}' for #{item.name}, must be one of #{item.states.keys.join(', ')}"
           else
             raise e
           end

openc3/python/openc3/api/tlm_api.py

@@ -132,7 +132,15 @@ def inject_tlm(target_name, packet_name, item_hash=None, type="CONVERTED", scope
     if item_hash:
         item_hash = {k.upper(): v for k, v in item_hash.items()}
         # Check that the items exist ... exceptions are raised if not
-        TargetModel.packet_items(target_name, packet_name, item_hash.keys(), scope=scope)
+        items = TargetModel.packet_items(target_name, packet_name, item_hash.keys(), scope=scope)
+        if type == 'CONVERTED':
+            # If the type is converted, check that the item states are valid
+            for item_name, item_value in item_hash.items():
+                item = next((i for i in items if i['name'] == item_name.upper()), None)
+                if item and item.get('states') and item_value not in item['states']:
+                    raise RuntimeError(
+                        f"Unknown state '{item_value}' for {item['name']}, must be one of {', '.join(item['states'].keys())}"
+                    )
     else:
         # Check that the packet exists ... exceptions are raised if not
         TargetModel.packet(target_name, packet_name, scope=scope)

openc3/python/openc3/packets/packet.py

@@ -655,7 +655,7 @@ def write_item(self, item, value, value_type="CONVERTED", buffer=None):
                     super().write_item(item, value, "RAW", buffer)
                 except ValueError as error:
                     if item.states and isinstance(value, str) and "invalid literal for" in repr(error):
-                        raise ValueError(f"Unknown state {value} for {item.name}, must be one of f{', '.join(item.states.keys())}") from error
+                        raise ValueError(f"Unknown state '{value}' for {item.name}, must be one of f{', '.join(item.states.keys())}") from error
                     else:
                         raise error
             case "FORMATTED" | "WITH_UNITS":

openc3/python/test/api/test_limits_api.py

@@ -424,8 +424,8 @@ def test_get_overall_limits_state_returns_the_overall_system_limits_state(self):
                 "TEMP2": 0,
                 "TEMP3": 0,
                 "TEMP4": 0,
-                "GROUND1STATUS": 1,
-                "GROUND2STATUS": 1,
+                "GROUND1STATUS": 'CONNECTED',
+                "GROUND2STATUS": 'CONNECTED',
             },
         )
         time.sleep(0.1)

openc3/python/test/packets/test_packet.py

@@ -684,7 +684,7 @@ def test_writes_the_converted_value_with_states(self):
         self.assertEqual(self.buffer, b"\x01\x00\x00\x00")
         self.p.write_item(i, "FALSE", "CONVERTED", self.buffer)
         self.assertEqual(self.buffer, b"\x02\x00\x00\x00")
-        with self.assertRaisesRegex(ValueError, "Unknown state BLAH for ITEM"):
+        with self.assertRaisesRegex(ValueError, "Unknown state 'BLAH' for ITEM"):
             self.p.write_item(i, "BLAH", "CONVERTED", self.buffer)
         i.write_conversion = GenericConversion("value / 2")
         self.p.write("ITEM", 4, "CONVERTED", self.buffer)

openc3/spec/api/cmd_api_spec.rb

@@ -168,7 +168,7 @@ def test_cmd_unknown(method)
               expect { @api.send(method, "INST COLLECT with TYPE #{type}, DURATION 10") }.not_to raise_error
             else
               # Non-raw commands still raise because the state parameter is checked during the write
-              expect { @api.send(method, "INST COLLECT with TYPE #{type}, DURATION 10") }.to raise_error("Unknown state OTHER for TYPE, must be one of NORMAL, SPECIAL")
+              expect { @api.send(method, "INST COLLECT with TYPE #{type}, DURATION 10") }.to raise_error("Unknown state 'OTHER' for TYPE, must be one of NORMAL, SPECIAL")
             end
           else
             # cmd(), cmd_raw() and (no_hazardous_check variants) check the state parameter and raise

openc3/spec/api/limits_api_spec.rb

@@ -308,7 +308,7 @@ class ApiTest
     describe "get_overall_limits_state" do
       it "returns the overall system limits state" do
         @api.inject_tlm("INST", "HEALTH_STATUS",
-                        { 'TEMP1' => 0, 'TEMP2' => 0, 'TEMP3' => 0, 'TEMP4' => 0, 'GROUND1STATUS' => 1, 'GROUND2STATUS' => 1 })
+                        { 'TEMP1' => 0, 'TEMP2' => 0, 'TEMP3' => 0, 'TEMP4' => 0, 'GROUND1STATUS' => 'CONNECTED', 'GROUND2STATUS' => 'CONNECTED' })
         sleep 0.1
         expect(@api.get_overall_limits_state).to eql "GREEN"
         # TEMP1 limits: -80.0 -70.0 60.0 80.0 -20.0 20.0

openc3/spec/api/tlm_api_spec.rb

@@ -40,6 +40,7 @@ class ApiTest
     before(:each) do
       mock_redis()
       setup_system()
+      local_s3()
 
       %w(INST SYSTEM).each do |target|
         model = TargetModel.new(folder_name: target, name: target, scope: "DEFAULT")
@@ -57,6 +58,7 @@ class ApiTest
     end
 
     after(:each) do
+      local_s3_unset()
       Thread.list.each do |t|
         t.join if t != Thread.current
       end

openc3/spec/packets/packet_spec.rb

@@ -729,7 +729,7 @@ module OpenC3
         expect(@buffer).to eql "\x01\x00\x00\x00"
         @p.write_item(i, "FALSE", :CONVERTED, @buffer)
         expect(@buffer).to eql "\x02\x00\x00\x00"
-        expect { @p.write_item(i, "BLAH", :CONVERTED, @buffer) }.to raise_error(RuntimeError, "Unknown state BLAH for ITEM, must be one of TRUE, FALSE")
+        expect { @p.write_item(i, "BLAH", :CONVERTED, @buffer) }.to raise_error(RuntimeError, "Unknown state 'BLAH' for ITEM, must be one of TRUE, FALSE")
         i.write_conversion = GenericConversion.new("value / 2")
         @p.write("ITEM", 4, :CONVERTED, @buffer)
         expect(@buffer).to eql "\x02\x00\x00\x00"