cmd and cmd_raw raise on bad item states

openc3/lib/openc3/packets/commands.rb

@@ -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 2024, OpenC3, Inc.
 # All Rights Reserved
 #
 # This file may also be used under the terms of a commercial license
@@ -308,12 +308,21 @@ def set_parameters(command, params, range_checking)
         item = command.get_item(item_upcase)
         range_check_value = value
 
-        # Convert from state to value if possible
-        if item.states and item.states[value.to_s.upcase]
-          range_check_value = item.states[value.to_s.upcase]
-        end
-
         if range_checking
+          if item.states
+            if item.states[value.to_s.upcase]
+              range_check_value = item.states[value.to_s.upcase]
+            else
+              if command.raw
+                unless item.states.values.include?(value)
+                  raise "Command parameter '#{command.target_name} #{command.packet_name} #{item_upcase}' = #{value.to_s.upcase} not one of #{item.states.values.join(', ')}"
+                end
+              else
+                raise "Command parameter '#{command.target_name} #{command.packet_name} #{item_upcase}' = #{value.to_s.upcase} not one of #{item.states.keys.join(', ')}"
+              end
+            end
+          end
+
           range = item.range
           if range
             # Perform Range Check on command parameter

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}"
+            raise "Unknown state #{value} for #{item.name}, must be one of #{item.states.keys.join(', ')}"
           else
             raise e
           end

openc3/spec/api/cmd_api_spec.rb

@@ -42,6 +42,7 @@ class ApiTest
     before(:each) do
       redis = mock_redis()
       setup_system()
+      local_s3()
 
       require 'openc3/models/target_model'
       model = TargetModel.new(folder_name: 'INST', name: 'INST', scope: "DEFAULT")
@@ -83,6 +84,7 @@ class ApiTest
     end
 
     after(:each) do
+      local_s3_unset()
       InterfaceTopic.shutdown(@interface, scope: 'DEFAULT')
       count = 0
       while @int_thread.alive? or count < 100 do
@@ -94,10 +96,8 @@ class ApiTest
     def test_cmd_unknown(method)
       expect { @api.send(method, "BLAH COLLECT with TYPE NORMAL") }.to raise_error(/does not exist/)
       expect { @api.send(method, "INST UNKNOWN with TYPE NORMAL") }.to raise_error(/does not exist/)
-      # expect { @api.send(method, "INST COLLECT with BLAH NORMAL") }.to raise_error(/does not exist/)
       expect { @api.send(method, "BLAH", "COLLECT", "TYPE" => "NORMAL") }.to raise_error(/does not exist/)
       expect { @api.send(method, "INST", "UNKNOWN", "TYPE" => "NORMAL") }.to raise_error(/does not exist/)
-      # expect { @api.send(method, "INST", "COLLECT", "BLAH"=>"NORMAL") }.to raise_error(/does not exist/)
     end
 
     %w(cmd cmd_no_checks cmd_no_range_check cmd_no_hazardous_check cmd_raw cmd_raw_no_checks cmd_raw_no_range_check cmd_raw_no_hazardous_check).each do |method|
@@ -154,6 +154,28 @@ def test_cmd_unknown(method)
           end
         end
 
+        it "warns about bad state parameters" do
+          if method.include?('raw')
+            type = 2
+            check = '0, 1'
+          else
+            type = 'OTHER'
+            check = 'NORMAL, SPECIAL'
+          end
+          if method.include?('no_checks') or method.include?('no_range')
+            if method.include?('raw')
+              # If we're using raw commands, we can set any state parameter because it's numeric
+              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")
+            end
+          else
+            # cmd(), cmd_raw() and (no_hazardous_check variants) check the state parameter and raise
+            expect { @api.send(method, "INST COLLECT with TYPE #{type}, DURATION 10") }.to raise_error("Command parameter 'INST COLLECT TYPE' = #{type} not one of #{check}")
+          end
+        end
+
         it "warns about hazardous parameters" do
           type = method.include?('raw') ? 1 : 'SPECIAL'
           if method.include?('no_checks') or method.include?('no_hazard')

openc3/spec/models/cvt_model_spec.rb

@@ -47,6 +47,9 @@ def check_temp1
       mock_redis()
       setup_system()
       local_s3()
+    end
+
+    after(:all) do
       local_s3_unset()
     end
 

openc3/spec/models/microservice_model_spec.rb

@@ -27,6 +27,11 @@ module OpenC3
   describe MicroserviceModel do
     before(:each) do
       mock_redis()
+      local_s3()
+    end
+
+    after(:each) do
+      local_s3_unset()
     end
 
     describe "self.get" do

openc3/spec/models/plugin_model_spec.rb

@@ -293,6 +293,14 @@ module OpenC3
     end
 
     describe "self.undeploy" do
+      before(:each) do
+        local_s3()
+      end
+
+      after(:each) do
+        local_s3_unset()
+      end
+
       it "destroys all models associated with the plugin" do
         tool = ToolModel.new(name: "TOOL", folder_name: "TOOL", scope: "DEFAULT", plugin: "PLUG")
         tool.create

openc3/spec/packets/commands_spec.rb

@@ -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 2024, OpenC3, Inc.
 # All Rights Reserved
 #
 # This file may also be used under the terms of a commercial license
@@ -278,6 +278,10 @@ module OpenC3
         expect { @cmd.build_cmd("tgt1", "pkt1", { "item2" => 1000 }) }.to raise_error(RuntimeError, "Command parameter 'TGT1 PKT1 ITEM2' = 1000 not in valid range of 0 to 254")
       end
 
+      it "complains about out of range item states" do
+        expect { @cmd.build_cmd("tgt1", "pkt2", { "item2" => "OTHER" }) }.to raise_error(RuntimeError, "Command parameter 'TGT1 PKT2 ITEM2' = OTHER not one of BAD1, BAD2, GOOD")
+      end
+
       it "ignores out of range item values if requested" do
         cmd = @cmd.build_cmd("tgt1", "pkt1", { "item2" => 255 }, false)
         cmd.enable_method_missing

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")
+        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"

playwright/tests/command-sender.spec.ts

@@ -104,33 +104,6 @@ test('displays parameter units, ranges and description', async ({
   await expect(row.locator('td >> nth=4')).toContainText('Collect temperature')
 })
 
-test('supports manually entered state values', async ({ page, utils }) => {
-  await page.locator('[data-test="clear-history"]').click()
-  await utils.selectTargetPacketItem('INST', 'COLLECT')
-  await setValue(page, 'TYPE', '3')
-  // Typing in the state value should automatically switch the state
-  await expect(page.locator('tr:has-text("TYPE")')).toContainText(
-    'MANUALLY ENTERED',
-  )
-
-  // Manually typing in an existing state value should change the state drop down
-  await setValue(page, 'TYPE', '0x0')
-  await expect(page.locator('tr:has-text("TYPE")')).toContainText('NORMAL')
-  await setValue(page, 'TYPE', '1')
-  await expect(page.locator('tr:has-text("TYPE")')).toContainText('SPECIAL')
-  // Switch back to MANUALLY ENTERED
-  await selectValue(page, 'TYPE', 'MANUALLY ENTERED')
-  await setValue(page, 'TYPE', '3')
-  await page.locator('[data-test="select-send"]').click()
-  await expect(page.locator('main')).toContainText(
-    'cmd("INST COLLECT with TYPE 3, DURATION 1, OPCODE 171, TEMP 0") sent',
-  )
-  await checkHistory(
-    page,
-    'cmd("INST COLLECT with TYPE 3, DURATION 1, OPCODE 171, TEMP 0")',
-  )
-})
-
 test('warns for hazardous commands', async ({ page, utils }) => {
   await page.locator('[data-test="clear-history"]').click()
   await utils.selectTargetPacketItem('INST', 'CLEAR')
@@ -181,7 +154,9 @@ test('warns for required parameters', async ({ page, utils }) => {
   // Break apart the checks so we have output flexibility in the future
   await expect(page.locator('.v-dialog')).toContainText('Error sending')
   await expect(page.locator('.v-dialog')).toContainText('INST COLLECT TYPE')
-  await expect(page.locator('.v-dialog')).toContainText('not in valid range')
+  await expect(page.locator('.v-dialog')).toContainText(
+    'not one of NORMAL, SPECIAL',
+  )
   await page.locator('button:has-text("Ok")').click()
 })
 
@@ -293,23 +268,13 @@ test('handles string values', async ({ page, utils }) => {
   await expect(page.locator('main')).toContainText(
     "cmd(\"INST ASCIICMD with STRING 'ARM LASER', BINARY 0xDEADBEEF, ASCII '0xDEADBEEF'\")",
   )
-  // Enter a custom string
-  await setValue(page, 'STRING', 'MY VAL')
   // Enter a custom binary value
+  await selectValue(page, 'STRING', 'NOOP')
   await setValue(page, 'BINARY', '0xBA5EBA11')
-  // Typing in the state value should automatically switch the state
-  await expect(page.locator('tr:has-text("STRING")').first()).toContainText(
-    'MANUALLY ENTERED',
-  )
   await page.locator('[data-test="select-send"]').click()
   await expect(page.locator('main')).toContainText(
-    "cmd(\"INST ASCIICMD with STRING 'MY VAL', BINARY 0xBA5EBA11, ASCII '0xDEADBEEF'\")",
+    "cmd(\"INST ASCIICMD with STRING 'NOOP', BINARY 0xBA5EBA11, ASCII '0xDEADBEEF'\")",
   )
-  // Manually typing in an existing state value should change the state drop down
-  await setValue(page, 'STRING', 'FIRE LASER')
-  await expect(
-    page.locator('div[role=button]:has-text("FIRE LASER")'),
-  ).toBeVisible()
 })
 
 test('gets details with right click', async ({ page, utils }) => {