From 1962ddb11bbabf1ebea2871336ca2d7408a46053 Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Mon, 12 Oct 2026 16:11:46 -0600 Subject: [PATCH 01/14] fix(telemetry): handle LATEST in streaming dedupe and get_limits - Include item_key in StreamingObject#id so a LATEST item no longer collides with an explicit subscription to the same packet item - Fall back to removing by key in StreamingObjectCollection#remove since items are added as [key, item_key] but removed by key alone - Resolve LATEST in get_limits using the caller's scope, not $openc3_scope, and inline the single-use _get_item helper - Authorize the resolved packet in get_limits (Ruby and Python) instead of the literal LATEST, and upcase the names Co-Authored-By: Claude Opus 5.5 --- .../app/models/streaming_object.rb | 4 +- .../app/models/streaming_object_collection.rb | 63 +++++++++++-------- .../spec/models/streaming_api_spec.rb | 50 +++++++++++++++ openc3/lib/openc3/api/limits_api.rb | 22 +++---- openc3/python/openc3/api/limits_api.py | 22 +++---- openc3/python/test/api/test_limits_api.py | 8 +++ openc3/spec/api/limits_api_spec.rb | 15 +++++ 7 files changed, 128 insertions(+), 56 deletions(-) diff --git a/openc3-cosmos-cmd-tlm-api/app/models/streaming_object.rb b/openc3-cosmos-cmd-tlm-api/app/models/streaming_object.rb index 7ecffe61d8..8c92337111 100644 --- a/openc3-cosmos-cmd-tlm-api/app/models/streaming_object.rb +++ b/openc3-cosmos-cmd-tlm-api/app/models/streaming_object.rb @@ -76,7 +76,9 @@ def initialize(key, start_time_nsec, end_time_nsec, item_key: nil, scope:, token @offset = "0-0" @offset = OpenC3::Topic.get_last_offset(@topic, db_shard: @db_shard) unless @start_time if @item_key - @id = 'ITEM__' + key + # Include the item_key so a LATEST item (which expands to concrete packet keys) + # doesn't collide with an explicit subscription to the same packet item + @id = "ITEM__#{key}__#{@item_key}" else @id = 'PACKET__' + key end diff --git a/openc3-cosmos-cmd-tlm-api/app/models/streaming_object_collection.rb b/openc3-cosmos-cmd-tlm-api/app/models/streaming_object_collection.rb index c2c7b5e5ca..890cf03906 100644 --- a/openc3-cosmos-cmd-tlm-api/app/models/streaming_object_collection.rb +++ b/openc3-cosmos-cmd-tlm-api/app/models/streaming_object_collection.rb @@ -59,35 +59,46 @@ def remove(object) @mutex.synchronize do found_object = @objects_by_id[object.id] if found_object - @objects.delete(found_object) - @objects_by_id.delete(found_object.id) - item_objects = @item_objects_by_topic[object.topic] - item_objects.delete(found_object) if item_objects - packet_objects = @packet_objects_by_topic[object.topic] - packet_objects.delete(found_object) if packet_objects - if item_objects - if item_objects.length == 0 - if packet_objects - if packet_objects.length == 0 - # Nothing left in either for this topic - @topics_and_offsets.delete(object.topic) - end - else - # Just item_objects and nothing left - @topics_and_offsets.delete(object.topic) - end - end - else - if packet_objects - if packet_objects.length == 0 - # Just packet_objects and nothing left - @topics_and_offsets.delete(object.topic) - end - else - # Neither objects - Shouldn't happen + _remove_found(found_object) + elsif object.item_key + # Items can be added as [key, item_key] pairs but removed by key alone, + # so fall back to removing every item object with a matching key + @objects.select { |obj| obj.item_key and obj.key == object.key }.each do |obj| + _remove_found(obj) + end + end + end + end + + # Must be called with @mutex held + def _remove_found(object) + @objects.delete(object) + @objects_by_id.delete(object.id) + item_objects = @item_objects_by_topic[object.topic] + item_objects.delete(object) if item_objects + packet_objects = @packet_objects_by_topic[object.topic] + packet_objects.delete(object) if packet_objects + if item_objects + if item_objects.length == 0 + if packet_objects + if packet_objects.length == 0 + # Nothing left in either for this topic @topics_and_offsets.delete(object.topic) end + else + # Just item_objects and nothing left + @topics_and_offsets.delete(object.topic) + end + end + else + if packet_objects + if packet_objects.length == 0 + # Just packet_objects and nothing left + @topics_and_offsets.delete(object.topic) end + else + # Neither objects - Shouldn't happen + @topics_and_offsets.delete(object.topic) end end end diff --git a/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb b/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb index c89f6767e2..9c463f2c6d 100644 --- a/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb +++ b/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb @@ -498,6 +498,56 @@ def response.is_truncated end end + describe '#build_item_collection' do + before(:each) do + allow(OpenC3::TargetModel).to receive(:get_item_to_packet_map).with('INST', scope: 'DEFAULT').and_return({ + 'TEMP1' => ['HEALTH_STATUS', 'PARAMS'], + }) + end + + let(:latest_key) { 'DECOM__TLM__INST__LATEST__TEMP1__CONVERTED' } + let(:explicit_key) { 'DECOM__TLM__INST__HEALTH_STATUS__TEMP1__CONVERTED' } + + def build(items) + collection = StreamingObjectCollection.new + @api.build_item_collection(collection, { 'items' => items }, scope: 'DEFAULT') + collection + end + + it 'expands LATEST into one object per packet keyed by the LATEST key' do + collection = build([latest_key]) + expect(collection.objects.map { |o| [o.key, o.item_key] }).to eq([ + ['DECOM__TLM__INST__HEALTH_STATUS__TEMP1__CONVERTED', latest_key], + ['DECOM__TLM__INST__PARAMS__TEMP1__CONVERTED', latest_key], + ]) + end + + it 'keeps a LATEST item and an explicit item for the same packet' do + collection = build([latest_key, explicit_key]) + expect(collection.length).to eq(3) + expect(collection.objects.select { |o| o.key == explicit_key }.map(&:item_key)).to contain_exactly(latest_key, explicit_key) + end + + it 'removing the LATEST item leaves the explicit item' do + collection = build([latest_key, explicit_key]) + build([latest_key]).objects.each { |o| collection.remove(o) } + expect(collection.objects.map { |o| [o.key, o.item_key] }).to eq([[explicit_key, explicit_key]]) + end + + it 'removing the explicit item leaves the LATEST item' do + collection = build([latest_key, explicit_key]) + build([explicit_key]).objects.each { |o| collection.remove(o) } + expect(collection.objects.map(&:item_key).uniq).to eq([latest_key]) + expect(collection.length).to eq(2) + end + + it 'removes an item added with an item_key when removed by key alone' do + collection = build([[explicit_key, '0']]) + build([explicit_key]).objects.each { |o| collection.remove(o) } + expect(collection).to be_empty + end + end + describe 'add with COSMOS_ALL packets and authorization' do it 'skips unauthorized packets when using COSMOS_ALL' do $openc3_authorize = true diff --git a/openc3/lib/openc3/api/limits_api.rb b/openc3/lib/openc3/api/limits_api.rb index d233536de0..b2427f0870 100644 --- a/openc3/lib/openc3/api/limits_api.rb +++ b/openc3/lib/openc3/api/limits_api.rb @@ -189,9 +189,16 @@ def disable_limits(*args, manual: false, scope: $openc3_scope, token: $openc3_to # # @return [Hash{String => Array}] def get_limits(target_name, packet_name, item_name, cache_timeout: nil, manual: false, scope: $openc3_scope, token: $openc3_token) + target_name = target_name.upcase + packet_name = packet_name.upcase + item_name = item_name.upcase + # Resolve LATEST before authorizing so the real packet is authorized + if packet_name == 'LATEST' + packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout: cache_timeout, scope: scope) + end authorize(permission: 'tlm', target_name: target_name, packet_name: packet_name, manual: manual, scope: scope, token: token) limits = {} - item = _get_item(target_name, packet_name, item_name, cache_timeout: cache_timeout, scope: scope) + item = TargetModel.packet_item(target_name, packet_name, item_name, scope: scope) item['limits'].each do |key, vals| next unless vals.is_a?(Hash) @@ -447,18 +454,5 @@ def _limits_group(group_name, action:, manual:, scope:, token:) TargetModel.set_packet(last_target_name, last_packet_name, packet, scope: scope) end end - - # Gets an item. The code below is mostly duplicated from tlm_process_args in tlm_api.rb. - # - # @param target_name [String] target name - # @param packet_name [String] packet name - # @param item_name [String] item name - # @param scope [String] scope - # @return Hash The requested item based on the packet name - def _get_item(target_name, packet_name, item_name, cache_timeout: nil, scope:) - # Determine if this item exists, it will raise appropriate errors if not - packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout: cache_timeout, scope: $openc3_scope) if packet_name == 'LATEST' - return TargetModel.packet_item(target_name, packet_name, item_name, scope: scope) - end end end diff --git a/openc3/python/openc3/api/limits_api.py b/openc3/python/openc3/api/limits_api.py index 18e68fb6b7..bcbd896d64 100644 --- a/openc3/python/openc3/api/limits_api.py +++ b/openc3/python/openc3/api/limits_api.py @@ -218,9 +218,15 @@ def disable_limits(*args, scope=OPENC3_SCOPE): # # @return [Hash{String => Array}] def get_limits(target_name, packet_name, item_name, scope=OPENC3_SCOPE): + target_name = target_name.upper() + packet_name = packet_name.upper() + item_name = item_name.upper() + # Resolve LATEST before authorizing so the real packet is authorized + if packet_name == "LATEST": + packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, scope=scope) authorize(permission="tlm", target_name=target_name, packet_name=packet_name, scope=scope) limits = {} - item = _get_item(target_name, packet_name, item_name, scope=scope) + item = TargetModel.packet_item(target_name, packet_name, item_name, scope=scope) for key, vals in item["limits"].items(): if not isinstance(vals, dict): continue @@ -529,17 +535,3 @@ def _limits_group(group_name, action, scope): last_packet_name = packet_name if last_target_name and last_packet_name: TargetModel.set_packet(last_target_name, last_packet_name, packet, scope=scope) - - -# Gets an item. The code below is mostly duplicated from tlm_process_args in tlm_api.rb. -# -# @param target_name [String] target name -# @param packet_name [String] packet name -# @param item_name [String] item name -# @param scope [String] scope -# @return Hash The requested item based on the packet name -def _get_item(target_name, packet_name, item_name, cache_timeout=0.1, scope=OPENC3_SCOPE): - # Determine if this item exists, it will raise appropriate errors if not - if packet_name == "LATEST": - packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout, scope) - return TargetModel.packet_item(target_name, packet_name, item_name, scope=scope) diff --git a/openc3/python/test/api/test_limits_api.py b/openc3/python/test/api/test_limits_api.py index 25d8794ed7..749a6c9a5d 100644 --- a/openc3/python/test/api/test_limits_api.py +++ b/openc3/python/test/api/test_limits_api.py @@ -117,6 +117,14 @@ def test_gets_limits_for_a_latest_item(self): }, ) + @patch("openc3.api.limits_api.authorize") + def test_get_limits_authorizes_the_resolved_packet_rather_than_latest(self, mock_authorize): + with patch.object(CvtModel, "determine_latest_packet_for_item", return_value="HEALTH_STATUS"): + get_limits("inst", "latest", "temp1") + mock_authorize.assert_called_once_with( + permission="tlm", target_name="INST", packet_name="HEALTH_STATUS", scope="DEFAULT" + ) + def test_set_limits_complains_about_non_existant_targets(self): with self.assertRaisesRegex(RuntimeError, "Packet definition 'BLAH HEALTH_STATUS' does not exist"): set_limits("BLAH", "HEALTH_STATUS", "TEMP1", 0.0, 10.0, 20.0, 30.0) diff --git a/openc3/spec/api/limits_api_spec.rb b/openc3/spec/api/limits_api_spec.rb index fb081cd8fd..285617109a 100644 --- a/openc3/spec/api/limits_api_spec.rb +++ b/openc3/spec/api/limits_api_spec.rb @@ -90,6 +90,21 @@ def with_decom_microservice eql({ 'DEFAULT' => [-80.0, -70.0, 60.0, 80.0, -20.0, 20.0], 'TVAC' => [-80.0, -30.0, 30.0, 80.0] }) end end + + it "resolves a LATEST item in the requested scope" do + model = TargetModel.new(folder_name: 'INST', name: 'INST', scope: "OTHER") + model.create + model.update_store(System.new(['INST'], File.join(SPEC_DIR, 'install', 'config', 'targets'))) + expect(CvtModel).to receive(:determine_latest_packet_for_item).with('INST', 'TEMP1', cache_timeout: nil, scope: 'OTHER').and_call_original + expect(@api.get_limits("INST", "LATEST", "TEMP1", scope: "OTHER")).to \ + eql({ 'DEFAULT' => [-80.0, -70.0, 60.0, 80.0, -20.0, 20.0], 'TVAC' => [-80.0, -30.0, 30.0, 80.0] }) + end + + it "authorizes the resolved packet rather than LATEST" do + allow(CvtModel).to receive(:determine_latest_packet_for_item).and_return('HEALTH_STATUS') + expect(@api).to receive(:authorize).with(hash_including(target_name: 'INST', packet_name: 'HEALTH_STATUS')) + @api.get_limits("inst", "latest", "temp1") + end end describe "set_limits" do From 2fc52acd28c19b1a422b1c45dd8c6b620f9b0e90 Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Mon, 12 Oct 2026 16:23:33 -0600 Subject: [PATCH 02/14] fix(telemetry): cache item-to-packet map per scope - Key TargetModel item_map_cache by the scoped item_map_key instead of the target name so LATEST can't resolve against another scope's map - Clear the cache entry in test_cvt_model so the LATEST test no longer depends on test order Co-Authored-By: Claude Opus 5.5 --- openc3/lib/openc3/models/target_model.rb | 11 ++++++----- openc3/python/openc3/models/target_model.py | 11 ++++++----- openc3/python/test/models/test_cvt_model.py | 2 ++ openc3/python/test/models/test_target_model.py | 17 +++++++++++++++-- openc3/spec/models/cvt_model_spec.rb | 2 +- openc3/spec/models/target_model_spec.rb | 11 +++++++++-- 6 files changed, 39 insertions(+), 15 deletions(-) diff --git a/openc3/lib/openc3/models/target_model.rb b/openc3/lib/openc3/models/target_model.rb index 090c353fc5..a5c7e5e73d 100644 --- a/openc3/lib/openc3/models/target_model.rb +++ b/openc3/lib/openc3/models/target_model.rb @@ -429,9 +429,10 @@ def self.limits_groups(scope:) end def self.get_item_to_packet_map(target_name, scope:) - cache_time, item_map = @@item_map_cache[target_name] - return item_map if item_map and (Time.now - cache_time) < ITEM_MAP_CACHE_TIMEOUT + # The cache is keyed by item_map_key since the same target can exist in multiple scopes item_map_key = "#{scope}__#{target_name}__item_to_packet_map" + cache_time, item_map = @@item_map_cache[item_map_key] + return item_map if item_map and (Time.now - cache_time) < ITEM_MAP_CACHE_TIMEOUT target_name = target_name.upcase store = store_for_target(target_name, scope: scope) json_data = store.get(item_map_key) @@ -441,7 +442,7 @@ def self.get_item_to_packet_map(target_name, scope:) item_map = build_item_to_packet_map(target_name, scope: scope) store.set(item_map_key, JSON.generate(item_map, allow_nan: true)) end - @@item_map_cache[target_name] = [Time.now, item_map] + @@item_map_cache[item_map_key] = [Time.now, item_map] return item_map end @@ -886,7 +887,7 @@ def undeploy # Delete item_map item_map_key = "#{@scope}__#{@name}__item_to_packet_map" db_shard_store.del(item_map_key) - @@item_map_cache[@name] = nil + @@item_map_cache[item_map_key] = nil topic = { kind: 'deleted', type: 'target', name: @name } # The UNKNOWN target doesn't have an associated plugin @@ -1049,7 +1050,7 @@ def update_store_item_map item_map_key = "#{@scope}__#{@name}__item_to_packet_map" item_map = self.class.build_item_to_packet_map(@name, scope: @scope) Store.instance(db_shard: @db_shard).set(item_map_key, JSON.generate(item_map, allow_nan: true)) - @@item_map_cache[@name] = [Time.now, item_map] + @@item_map_cache[item_map_key] = [Time.now, item_map] end def update_store(system, clear_old: true) diff --git a/openc3/python/openc3/models/target_model.py b/openc3/python/openc3/models/target_model.py index cd920ce644..88d2985479 100644 --- a/openc3/python/openc3/models/target_model.py +++ b/openc3/python/openc3/models/target_model.py @@ -228,11 +228,12 @@ def limits_groups(cls, scope: str = OPENC3_SCOPE): @classmethod def get_item_to_packet_map(cls, target_name: str, scope: str = OPENC3_SCOPE): - if target_name in TargetModel.item_map_cache: - cache_time, item_map = TargetModel.item_map_cache[target_name] + # The cache is keyed by item_map_key since the same target can exist in multiple scopes + item_map_key = f"{scope}__{target_name}__item_to_packet_map" + if item_map_key in TargetModel.item_map_cache: + cache_time, item_map = TargetModel.item_map_cache[item_map_key] if (time.time() - cache_time) < TargetModel.ITEM_MAP_CACHE_TIMEOUT: return item_map - item_map_key = f"{scope}__{target_name}__item_to_packet_map" target_name = target_name.upper() store = cls._store_for_target(target_name, scope) json_data = store.get(item_map_key) @@ -241,7 +242,7 @@ def get_item_to_packet_map(cls, target_name: str, scope: str = OPENC3_SCOPE): else: item_map = cls.build_item_to_packet_map(target_name, scope=scope) store.set(item_map_key, json.dumps(item_map)) - TargetModel.item_map_cache[target_name] = [time.time(), item_map] + TargetModel.item_map_cache[item_map_key] = [time.time(), item_map] return item_map @classmethod @@ -573,7 +574,7 @@ def update_store_item_map(self): item_map_key = f"{self.scope}__{self.name}__item_to_packet_map" item_map = TargetModel.build_item_to_packet_map(self.name, scope=self.scope) Store.instance(db_shard=self.db_shard).set(item_map_key, json.dumps(item_map)) - TargetModel.item_map_cache[self.name] = [time.time(), item_map] + TargetModel.item_map_cache[item_map_key] = [time.time(), item_map] def dynamic_update(self, packets, cmd_or_tlm="TELEMETRY", filename="dynamic_tlm.txt"): # Build hash of targets/packets diff --git a/openc3/python/test/models/test_cvt_model.py b/openc3/python/test/models/test_cvt_model.py index 8348817e73..67849c9e05 100644 --- a/openc3/python/test/models/test_cvt_model.py +++ b/openc3/python/test/models/test_cvt_model.py @@ -609,6 +609,8 @@ def test_determine_latest_packet_for_item_works_if_not_received(self): model.create() names = TargetModel.names(scope="DEFAULT") self.assertIn("INST", names) + # Clear the item_map cache to avoid interference from previous tests + TargetModel.item_map_cache.pop("DEFAULT__INST__item_to_packet_map", None) Store.set("DEFAULT__INST__item_to_packet_map", json.dumps({"PACKET_ID": ["PACKET1"]})) Store.hset("DEFAULT__tlm__INST", "PACKET1", json.dumps({"PACKET_TIMESECONDS": None})) packet_name = CvtModel.determine_latest_packet_for_item("INST", "PACKET_ID", scope="DEFAULT") diff --git a/openc3/python/test/models/test_target_model.py b/openc3/python/test/models/test_target_model.py index 776be35947..5d9a6f1bca 100644 --- a/openc3/python/test/models/test_target_model.py +++ b/openc3/python/test/models/test_target_model.py @@ -12,6 +12,7 @@ # A portion of this file was funded by Blue Origin Enterprises, L.P. # See https://github.com/OpenC3/cosmos/pull/1953 +import json import unittest import unittest.mock from unittest.mock import patch @@ -143,7 +144,16 @@ def test_gets_item_to_packet_map_from_cache(self): cache = dict(TargetModel.item_map_cache) TargetModel.get_item_to_packet_map("INST", scope="DEFAULT") # Verify the cache time doesn't change - self.assertEqual(cache["INST"][0], (cache["INST"][0])) + self.assertEqual( + cache["DEFAULT__INST__item_to_packet_map"][0], + TargetModel.item_map_cache["DEFAULT__INST__item_to_packet_map"][0], + ) + + def test_caches_the_item_to_packet_map_per_scope(self): + TargetModel.get_item_to_packet_map("INST", scope="DEFAULT") + Store.set("OTHER__INST__item_to_packet_map", json.dumps({"OTHER_ITEM": ["OTHER_PKT"]})) + self.assertEqual(TargetModel.get_item_to_packet_map("INST", scope="OTHER"), {"OTHER_ITEM": ["OTHER_PKT"]}) + self.assertIn("CCSDSVER", TargetModel.get_item_to_packet_map("INST", scope="DEFAULT")) def test_gets_item_to_packet_map_on_an_invalid_cache(self): TargetModel.get_item_to_packet_map("INST", scope="DEFAULT") @@ -153,7 +163,10 @@ def test_gets_item_to_packet_map_on_an_invalid_cache(self): TargetModel.ITEM_MAP_CACHE_TIMEOUT = 0 TargetModel.get_item_to_packet_map("INST", scope="DEFAULT") # Verify cached time was updated - self.assertLess(cache["INST"][0], TargetModel.item_map_cache["INST"][0]) + self.assertLess( + cache["DEFAULT__INST__item_to_packet_map"][0], + TargetModel.item_map_cache["DEFAULT__INST__item_to_packet_map"][0], + ) TargetModel.ITEM_MAP_CACHE_TIMEOUT = timeout def test_raises_for_an_unknown_type(self): diff --git a/openc3/spec/models/cvt_model_spec.rb b/openc3/spec/models/cvt_model_spec.rb index 00f62536a5..9719487de6 100644 --- a/openc3/spec/models/cvt_model_spec.rb +++ b/openc3/spec/models/cvt_model_spec.rb @@ -357,7 +357,7 @@ def check_temp1 names = TargetModel.names(scope:"DEFAULT") expect(names.include?("INST")) # Clear the item_map cache to avoid interference from previous tests - TargetModel.class_variable_get(:@@item_map_cache)["INST"] = nil + TargetModel.class_variable_get(:@@item_map_cache)["DEFAULT__INST__item_to_packet_map"] = nil Store.set("DEFAULT__INST__item_to_packet_map", JSON.generate({"PACKET_ID" => ["PACKET1"]})) Store.hset("DEFAULT__tlm__INST", "PACKET1", JSON.generate({"PACKET_TIMESECONDS" => nil})) packet_name = CvtModel.determine_latest_packet_for_item("INST", "PACKET_ID", scope:"DEFAULT") diff --git a/openc3/spec/models/target_model_spec.rb b/openc3/spec/models/target_model_spec.rb index 583c65e2c6..da750fe6c2 100644 --- a/openc3/spec/models/target_model_spec.rb +++ b/openc3/spec/models/target_model_spec.rb @@ -462,7 +462,14 @@ def shadow_response(body) expect(itpm["CCSDSVER"]).to eql(%w(ADCS HEALTH_STATUS HIDDEN IMAGE MECH PARAMS)) # Verify cached time was NOT updated cache = TargetModel.class_variable_get(:@@item_map_cache) - expect(cache["INST"][0]).to eq(orig_cache["INST"][0]) + expect(cache["DEFAULT__INST__item_to_packet_map"][0]).to eq(orig_cache["DEFAULT__INST__item_to_packet_map"][0]) + end + + it "caches the item-to-packet map per scope" do + TargetModel.get_item_to_packet_map("INST", scope: "DEFAULT") + Store.set("OTHER__INST__item_to_packet_map", JSON.generate({ "OTHER_ITEM" => ["OTHER_PKT"] })) + expect(TargetModel.get_item_to_packet_map("INST", scope: "OTHER")).to eql({ "OTHER_ITEM" => ["OTHER_PKT"] }) + expect(TargetModel.get_item_to_packet_map("INST", scope: "DEFAULT")["CCSDSVER"]).to be_a(Array) end it "gets item-to-packet map on an invalid cache" do @@ -477,7 +484,7 @@ def shadow_response(body) expect(itpm["CCSDSVER"]).to eql(%w(ADCS HEALTH_STATUS HIDDEN IMAGE MECH PARAMS)) # Verify cached time was updated cache = TargetModel.class_variable_get(:@@item_map_cache) - expect(cache["INST"][0]).to be > orig_cache["INST"][0] + expect(cache["DEFAULT__INST__item_to_packet_map"][0]).to be > orig_cache["DEFAULT__INST__item_to_packet_map"][0] OpenC3.disable_warnings do TargetModel::ITEM_MAP_CACHE_TIMEOUT = timeout end From 6000cc5e0d6806b77f621797e30b6132bc6d218d Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Mon, 12 Oct 2026 16:23:33 -0600 Subject: [PATCH 03/14] feat(telemetry): apply LATEST writes to every packet with the item - override_tlm, normalize_tlm, enable_limits and disable_limits expand LATEST to every packet containing the item via _tlm_process_args_all_packets - set_limits and set_state_color accept LATEST and validate every packet before changing any so a failure can't partially apply - Authorize each expanded packet before making any change - Factor _tlm_extract_args and _tlm_resolve_packet_name out of _tlm_process_args (Ruby and Python) Co-Authored-By: Claude Opus 5.5 --- openc3/lib/openc3/api/limits_api.rb | 232 +++++++-------- openc3/lib/openc3/api/tlm_api.rb | 87 ++++-- openc3/python/openc3/api/limits_api.py | 331 +++++++++++----------- openc3/python/openc3/api/tlm_api.py | 97 ++++--- openc3/python/test/api/test_limits_api.py | 31 ++ openc3/python/test/api/test_tlm_api.py | 10 + openc3/spec/api/limits_api_spec.rb | 37 +++ openc3/spec/api/tlm_api_spec.rb | 14 + 8 files changed, 493 insertions(+), 346 deletions(-) diff --git a/openc3/lib/openc3/api/limits_api.rb b/openc3/lib/openc3/api/limits_api.rb index b2427f0870..f7d4e15c51 100644 --- a/openc3/lib/openc3/api/limits_api.rb +++ b/openc3/lib/openc3/api/limits_api.rb @@ -121,29 +121,16 @@ def limits_enabled?(*args, manual: false, scope: $openc3_scope, token: $openc3_t # # Favor the first syntax where possible as it is more succinct. # - # @param args [String|Array] See the description for calling style + # @param args [String|Array] See the description for calling style. + # A packet name of LATEST applies to every packet which contains the item. def enable_limits(*args, manual: false, scope: $openc3_scope, token: $openc3_token) - target_name, packet_name, item_name = _tlm_process_args(args, 'enable_limits', scope: scope) - authorize(permission: 'tlm_set', target_name: target_name, packet_name: packet_name, manual: manual, scope: scope, token: token) - packet = TargetModel.packet(target_name, packet_name, scope: scope) - found_item = nil - packet['items'].each do |item| - if item['name'] == item_name - item['limits']['enabled'] = true - found_item = item - break - end + target_name, packet_names, item_name = _tlm_process_args_all_packets(args, 'enable_limits', scope: scope) + packet_names.each do |packet_name| + authorize(permission: 'tlm_set', target_name: target_name, packet_name: packet_name, manual: manual, scope: scope, token: token) + end + packet_names.each do |packet_name| + _set_limits_enabled(target_name, packet_name, item_name, true, scope: scope) end - raise "Item '#{target_name} #{packet_name} #{item_name}' does not exist while enabling limits" unless found_item - - TargetModel.set_packet(target_name, packet_name, packet, scope: scope) - - message = "Enabling Limits For '#{target_name} #{packet_name} #{item_name}'" - Logger.info(message, scope: scope) - - event = { type: :LIMITS_ENABLE_STATE, target_name: target_name, packet_name: packet_name, - item_name: item_name, enabled: true, time_nsec: Time.now.to_nsec_from_epoch, message: message } - LimitsEventTopic.write(event, scope: scope) end # Disable limit checking for a telemetry item @@ -154,29 +141,16 @@ def enable_limits(*args, manual: false, scope: $openc3_scope, token: $openc3_tok # # Favor the first syntax where possible as it is more succinct. # - # @param args [String|Array] See the description for calling style + # @param args [String|Array] See the description for calling style. + # A packet name of LATEST applies to every packet which contains the item. def disable_limits(*args, manual: false, scope: $openc3_scope, token: $openc3_token) - target_name, packet_name, item_name = _tlm_process_args(args, 'disable_limits', scope: scope) - authorize(permission: 'tlm_set', target_name: target_name, packet_name: packet_name, manual: manual, scope: scope, token: token) - packet = TargetModel.packet(target_name, packet_name, scope: scope) - found_item = nil - packet['items'].each do |item| - if item['name'] == item_name - item['limits']['enabled'] = false - found_item = item - break - end + target_name, packet_names, item_name = _tlm_process_args_all_packets(args, 'disable_limits', scope: scope) + packet_names.each do |packet_name| + authorize(permission: 'tlm_set', target_name: target_name, packet_name: packet_name, manual: manual, scope: scope, token: token) + end + packet_names.each do |packet_name| + _set_limits_enabled(target_name, packet_name, item_name, false, scope: scope) end - raise "Item '#{target_name} #{packet_name} #{item_name}' does not exist while disabling limits" unless found_item - - TargetModel.set_packet(target_name, packet_name, packet, scope: scope) - - message = "Disabling Limits for '#{target_name} #{packet_name} #{item_name}'" - Logger.info(message, scope: scope) - - event = { type: :LIMITS_ENABLE_STATE, target_name: target_name, packet_name: packet_name, - item_name: item_name, enabled: false, time_nsec: Time.now.to_nsec_from_epoch, message: message } - LimitsEventTopic.write(event, scope: scope) end # Get a Hash of all the limits sets defined for an item. Hash keys are the limit @@ -209,56 +183,58 @@ def get_limits(target_name, packet_name, item_name, cache_timeout: nil, manual: end # Change the limits settings for a given item. By default, a new limits set called 'CUSTOM' - # is created to avoid overriding existing limits. + # is created to avoid overriding existing limits. A packet name of LATEST + # applies to every packet which contains the item. def set_limits(target_name, packet_name, item_name, red_low, yellow_low, yellow_high, red_high, green_low = nil, green_high = nil, limits_set = 'CUSTOM', persistence = nil, enabled = true, manual: false, scope: $openc3_scope, token: $openc3_token) - authorize(permission: 'tlm_set', target_name: target_name, packet_name: packet_name, manual: manual, scope: scope, token: token) + target_name = target_name.upcase + packet_name = packet_name.upcase + item_name = item_name.upcase + packet_names = (packet_name == 'LATEST') ? _tlm_packet_names(target_name, packet_name, item_name, scope: scope) : [packet_name] + packet_names.each do |name| + authorize(permission: 'tlm_set', target_name: target_name, packet_name: name, manual: manual, scope: scope, token: token) + end if (red_low > yellow_low) || (yellow_low >= yellow_high) || (yellow_high > red_high) raise "Invalid limits specified. Ensure yellow limits are within red limits." end if (green_low && green_high) && ((yellow_low > green_low) || (green_low >= green_high) || (green_high > yellow_high)) raise "Invalid limits specified. Ensure green limits are within yellow limits." end - packet = TargetModel.packet(target_name, packet_name, scope: scope) - found_item = nil - packet['items'].each do |item| - if item['name'] == item_name - if item['limits'] - item['limits']['persistence_setting'] = persistence if persistence - if enabled - item['limits']['enabled'] = true - else - item['limits']['enabled'] = false - end - limits = {} - limits['red_low'] = red_low - limits['yellow_low'] = yellow_low - limits['yellow_high'] = yellow_high - limits['red_high'] = red_high - limits['green_low'] = green_low if green_low && green_high - limits['green_high'] = green_high if green_low && green_high - item['limits'][limits_set] = limits - found_item = item - break - else - raise "Cannot set_limits on item without any limits" - end - end + # Validate every packet before changing any so an error doesn't leave a partial update. + # TargetModel.packet returns a cached hash so it must not be modified until validated. + items = packet_names.map do |name| + packet = TargetModel.packet(target_name, name, scope: scope) + item = packet['items'].find { |packet_item| packet_item['name'] == item_name } + raise "Item '#{target_name} #{name} #{item_name}' does not exist while setting limits" unless item + raise "Cannot set_limits on item without any limits" unless item['limits'] + [name, packet, item] + end + items.each do |name, packet, item| + item['limits']['persistence_setting'] = persistence if persistence + item['limits']['enabled'] = enabled ? true : false + limits = {} + limits['red_low'] = red_low + limits['yellow_low'] = yellow_low + limits['yellow_high'] = yellow_high + limits['red_high'] = red_high + limits['green_low'] = green_low if green_low && green_high + limits['green_high'] = green_high if green_low && green_high + item['limits'][limits_set] = limits + + message = "Setting '#{target_name} #{name} #{item_name}' limits to #{red_low} #{yellow_low} #{yellow_high} #{red_high}" + message << " #{green_low} #{green_high}" if green_low && green_high + message << " in set #{limits_set} with persistence #{persistence} as enabled #{enabled}" + Logger.info(message, scope: scope) + + TargetModel.set_packet(target_name, name, packet, scope: scope) + + event = { type: :LIMITS_SETTINGS, target_name: target_name, packet_name: name, + item_name: item_name, red_low: red_low, yellow_low: yellow_low, yellow_high: yellow_high, red_high: red_high, + green_low: green_low, green_high: green_high, limits_set: limits_set, persistence: persistence, enabled: enabled, + time_nsec: Time.now.to_nsec_from_epoch, message: message } + LimitsEventTopic.write(event, scope: scope) end - raise "Item '#{target_name} #{packet_name} #{item_name}' does not exist while setting limits" unless found_item - message = "Setting '#{target_name} #{packet_name} #{item_name}' limits to #{red_low} #{yellow_low} #{yellow_high} #{red_high}" - message << " #{green_low} #{green_high}" if green_low && green_high - message << " in set #{limits_set} with persistence #{persistence} as enabled #{enabled}" - Logger.info(message, scope: scope) - - TargetModel.set_packet(target_name, packet_name, packet, scope: scope) - - event = { type: :LIMITS_SETTINGS, target_name: target_name, packet_name: packet_name, - item_name: item_name, red_low: red_low, yellow_low: yellow_low, yellow_high: yellow_high, red_high: red_high, - green_low: green_low, green_high: green_high, limits_set: limits_set, persistence: persistence, enabled: enabled, - time_nsec: Time.now.to_nsec_from_epoch, message: message } - LimitsEventTopic.write(event, scope: scope) end # Change the color associated with a telemetry item state in realtime. @@ -273,9 +249,16 @@ def set_limits(target_name, packet_name, item_name, red_low, yellow_low, yellow_ # or RED. Pass nil to clear (remove) the state color. Setting a color also # enables limits for the item, but clearing the color (nil) does not change # whether limits are enabled. + # A packet name of LATEST applies to every packet which contains the item. def set_state_color(target_name, packet_name, item_name, state_name, color, manual: false, scope: $openc3_scope, token: $openc3_token) - authorize(permission: 'tlm_set', target_name: target_name, packet_name: packet_name, manual: manual, scope: scope, token: token) + target_name = target_name.upcase + packet_name = packet_name.upcase + item_name = item_name.upcase + packet_names = (packet_name == 'LATEST') ? _tlm_packet_names(target_name, packet_name, item_name, scope: scope) : [packet_name] + packet_names.each do |name| + authorize(permission: 'tlm_set', target_name: target_name, packet_name: name, manual: manual, scope: scope, token: token) + end state_name = state_name.to_s.upcase unless color.nil? color = color.to_s.upcase @@ -283,40 +266,42 @@ def set_state_color(target_name, packet_name, item_name, state_name, color, raise "Invalid state color '#{color}'. Must be one of #{PacketItem::STATE_COLORS.join(', ')}." end end - packet = TargetModel.packet(target_name, packet_name, scope: scope) - found_item = nil - packet['items'].each do |item| - if item['name'] == item_name - unless item['states'] && item['states'][state_name] - raise "State '#{state_name}' does not exist for item '#{target_name} #{packet_name} #{item_name}'" - end - if color.nil? - # Clear the state color - item['states'][state_name].delete('color') - else - item['states'][state_name]['color'] = color - # Defining a state color implies limits are enabled for the item - item['limits'] ||= {} - item['limits']['enabled'] = true - end - found_item = item - break + # Validate every packet before changing any so an error doesn't leave a partial update. + # TargetModel.packet returns a cached hash so it must not be modified until validated. + items = packet_names.map do |name| + packet = TargetModel.packet(target_name, name, scope: scope) + item = packet['items'].find { |packet_item| packet_item['name'] == item_name } + raise "Item '#{target_name} #{name} #{item_name}' does not exist while setting state color" unless item + unless item['states'] && item['states'][state_name] + raise "State '#{state_name}' does not exist for item '#{target_name} #{name} #{item_name}'" end + [name, packet, item] end - raise "Item '#{target_name} #{packet_name} #{item_name}' does not exist while setting state color" unless found_item - if color.nil? - message = "Clearing '#{target_name} #{packet_name} #{item_name}' state #{state_name} color" - else - message = "Setting '#{target_name} #{packet_name} #{item_name}' state #{state_name} color to #{color}" - end - Logger.info(message, scope: scope) + items.each do |name, packet, item| + if color.nil? + # Clear the state color + item['states'][state_name].delete('color') + else + item['states'][state_name]['color'] = color + # Defining a state color implies limits are enabled for the item + item['limits'] ||= {} + item['limits']['enabled'] = true + end - TargetModel.set_packet(target_name, packet_name, packet, scope: scope) + if color.nil? + message = "Clearing '#{target_name} #{name} #{item_name}' state #{state_name} color" + else + message = "Setting '#{target_name} #{name} #{item_name}' state #{state_name} color to #{color}" + end + Logger.info(message, scope: scope) - event = { type: :LIMITS_STATE_COLOR, target_name: target_name, packet_name: packet_name, - item_name: item_name, state_name: state_name, color: color, - time_nsec: Time.now.to_nsec_from_epoch, message: message } - LimitsEventTopic.write(event, scope: scope) + TargetModel.set_packet(target_name, name, packet, scope: scope) + + event = { type: :LIMITS_STATE_COLOR, target_name: target_name, packet_name: name, + item_name: item_name, state_name: state_name, color: color, + time_nsec: Time.now.to_nsec_from_epoch, message: message } + LimitsEventTopic.write(event, scope: scope) + end end # Returns all limits_groups and their members @@ -454,5 +439,26 @@ def _limits_group(group_name, action:, manual:, scope:, token:) TargetModel.set_packet(last_target_name, last_packet_name, packet, scope: scope) end end + + def _set_limits_enabled(target_name, packet_name, item_name, enabled, scope:) + packet = TargetModel.packet(target_name, packet_name, scope: scope) + item = packet['items'].find { |packet_item| packet_item['name'] == item_name } + action = enabled ? 'enabling' : 'disabling' + raise "Item '#{target_name} #{packet_name} #{item_name}' does not exist while #{action} limits" unless item + + item['limits']['enabled'] = enabled + TargetModel.set_packet(target_name, packet_name, packet, scope: scope) + + if enabled + message = "Enabling Limits For '#{target_name} #{packet_name} #{item_name}'" + else + message = "Disabling Limits for '#{target_name} #{packet_name} #{item_name}'" + end + Logger.info(message, scope: scope) + + event = { type: :LIMITS_ENABLE_STATE, target_name: target_name, packet_name: packet_name, + item_name: item_name, enabled: enabled, time_nsec: Time.now.to_nsec_from_epoch, message: message } + LimitsEventTopic.write(event, scope: scope) + end end end diff --git a/openc3/lib/openc3/api/tlm_api.rb b/openc3/lib/openc3/api/tlm_api.rb index ee3d12352e..85095274d1 100644 --- a/openc3/lib/openc3/api/tlm_api.rb +++ b/openc3/lib/openc3/api/tlm_api.rb @@ -174,12 +174,17 @@ def inject_tlm(target_name, packet_name, item_hash = nil, type: :CONVERTED, stor # # @param args The args must either be a string followed by a value or # three strings followed by a value (see the calling style in the - # description). + # description). A packet name of LATEST overrides the item in every + # packet which contains it. # @param type [Symbol] Telemetry type, :ALL (default), :RAW, :CONVERTED, :FORMATTED def override_tlm(*args, type: :ALL, manual: false, scope: $openc3_scope, token: $openc3_token) - target_name, packet_name, item_name, value = _set_tlm_process_args(args, __method__, scope: scope) - authorize(permission: 'tlm_set', target_name: target_name, packet_name: packet_name, manual: manual, scope: scope, token: token) - CvtModel.override(target_name, packet_name, item_name, value, type: type.intern, scope: scope) + target_name, packet_names, item_name, value = _set_tlm_process_args_all_packets(args, __method__, scope: scope) + packet_names.each do |packet_name| + authorize(permission: 'tlm_set', target_name: target_name, packet_name: packet_name, manual: manual, scope: scope, token: token) + end + packet_names.each do |packet_name| + CvtModel.override(target_name, packet_name, item_name, value, type: type.intern, scope: scope) + end end # Get the list of CVT overrides @@ -198,13 +203,18 @@ def get_overrides(manual: false, scope: $openc3_scope, token: $openc3_token) # Favor the first syntax where possible as it is more succinct. # # @param args The args must either be a string or three strings - # (see the calling style in the description). + # (see the calling style in the description). A packet name of LATEST + # normalizes the item in every packet which contains it. # @param type [Symbol] Telemetry type, :ALL (default), :RAW, :CONVERTED, :FORMATTED # Also takes :ALL which means to normalize all telemetry types def normalize_tlm(*args, type: :ALL, manual: false, scope: $openc3_scope, token: $openc3_token) - target_name, packet_name, item_name = _tlm_process_args(args, __method__, scope: scope) - authorize(permission: 'tlm_set', target_name: target_name, packet_name: packet_name, manual: manual, scope: scope, token: token) - CvtModel.normalize(target_name, packet_name, item_name, type: type.intern, scope: scope) + target_name, packet_names, item_name = _tlm_process_args_all_packets(args, __method__, scope: scope) + packet_names.each do |packet_name| + authorize(permission: 'tlm_set', target_name: target_name, packet_name: packet_name, manual: manual, scope: scope, token: token) + end + packet_names.each do |packet_name| + CvtModel.normalize(target_name, packet_name, item_name, type: type.intern, scope: scope) + end end # Returns the raw buffer for a telemetry packet. @@ -593,6 +603,31 @@ def _validate_tlm_type(type) end def _tlm_process_args(args, method_name, cache_timeout: nil, scope: $openc3_scope, token: $openc3_token) + target_name, packet_name, item_name = _tlm_extract_args(args, method_name) + packet_name = _tlm_resolve_packet_name(target_name, packet_name, item_name, cache_timeout: cache_timeout, scope: scope) + return [target_name, packet_name, item_name] + end + + def _set_tlm_process_args(args, method_name, cache_timeout: nil, scope: $openc3_scope, token: $openc3_token) + target_name, packet_name, item_name, value = _set_tlm_extract_args(args, method_name) + packet_name = _tlm_resolve_packet_name(target_name, packet_name, item_name, cache_timeout: cache_timeout, scope: scope) + return [target_name, packet_name, item_name, value] + end + + # Like _tlm_process_args but returns an array of packet names where LATEST + # expands to every packet containing the item. Used by APIs which change + # an item so the change applies wherever the item appears. + def _tlm_process_args_all_packets(args, method_name, scope: $openc3_scope) + target_name, packet_name, item_name = _tlm_extract_args(args, method_name) + return [target_name, _tlm_packet_names(target_name, packet_name, item_name, scope: scope), item_name] + end + + def _set_tlm_process_args_all_packets(args, method_name, scope: $openc3_scope) + target_name, packet_name, item_name, value = _set_tlm_extract_args(args, method_name) + return [target_name, _tlm_packet_names(target_name, packet_name, item_name, scope: scope), item_name, value] + end + + def _tlm_extract_args(args, method_name) case args.length when 1 target_name, packet_name, item_name = extract_fields_from_tlm_text(args[0]) @@ -604,21 +639,10 @@ def _tlm_process_args(args, method_name, cache_timeout: nil, scope: $openc3_scop # Invalid number of arguments raise "ERROR: Invalid number of arguments (#{args.length}) passed to #{method_name}()" end - target_name = target_name.upcase - packet_name = packet_name.upcase - item_name = item_name.upcase - - if packet_name == 'LATEST' - packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout: cache_timeout, scope: scope) - else - # Determine if this item exists, it will raise appropriate errors if not - TargetModel.packet_item(target_name, packet_name, item_name, scope: scope) - end - - return [target_name, packet_name, item_name] + return [target_name.upcase, packet_name.upcase, item_name.upcase] end - def _set_tlm_process_args(args, method_name, cache_timeout: nil, scope: $openc3_scope, token: $openc3_token) + def _set_tlm_extract_args(args, method_name) case args.length when 1 target_name, packet_name, item_name, value = extract_fields_from_set_tlm_text(args[0]) @@ -631,18 +655,31 @@ def _set_tlm_process_args(args, method_name, cache_timeout: nil, scope: $openc3_ # Invalid number of arguments raise "ERROR: Invalid number of arguments (#{args.length}) passed to #{method_name}()" end - target_name = target_name.upcase - packet_name = packet_name.upcase - item_name = item_name.upcase + return [target_name.upcase, packet_name.upcase, item_name.upcase, value] + end + # Resolve LATEST to the most recently received packet containing the item + def _tlm_resolve_packet_name(target_name, packet_name, item_name, cache_timeout: nil, scope: $openc3_scope) if packet_name == 'LATEST' packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout: cache_timeout, scope: scope) else # Determine if this item exists, it will raise appropriate errors if not TargetModel.packet_item(target_name, packet_name, item_name, scope: scope) end + return packet_name + end - return [target_name, packet_name, item_name, value] + # Expand LATEST to every packet containing the item + def _tlm_packet_names(target_name, packet_name, item_name, scope: $openc3_scope) + if packet_name == 'LATEST' + packet_names = TargetModel.get_item_to_packet_map(target_name, scope: scope)[item_name] + raise "Item '#{target_name} LATEST #{item_name}' does not exist for scope: #{scope}" unless packet_names + return packet_names + else + # Determine if this item exists, it will raise appropriate errors if not + TargetModel.packet_item(target_name, packet_name, item_name, scope: scope) + return [packet_name] + end end end end diff --git a/openc3/python/openc3/api/limits_api.py b/openc3/python/openc3/api/limits_api.py index bcbd896d64..58bfb49d87 100644 --- a/openc3/python/openc3/api/limits_api.py +++ b/openc3/python/openc3/api/limits_api.py @@ -12,7 +12,7 @@ from datetime import datetime, timezone from openc3.api import WHITELIST -from openc3.api.tlm_api import _tlm_process_args +from openc3.api.tlm_api import _tlm_packet_names, _tlm_process_args, _tlm_process_args_all_packets from openc3.environment import OPENC3_SCOPE from openc3.models.cvt_model import CvtModel from openc3.models.target_model import TargetModel @@ -128,40 +128,14 @@ def limits_enabled(*args, scope=OPENC3_SCOPE): # # Favor the first syntax where possible as it is more succinct. # -# @param args [String|Array] See the description for calling style +# @param args [String|Array] See the description for calling style. +# A packet name of LATEST applies to every packet which contains the item. def enable_limits(*args, scope=OPENC3_SCOPE): - target_name, packet_name, item_name = _tlm_process_args(args, "enable_limits", scope=scope) - authorize( - permission="tlm_set", - target_name=target_name, - packet_name=packet_name, - scope=scope, - ) - packet = TargetModel.packet(target_name, packet_name, scope=scope) - found_item = None - for item in packet["items"]: - if item["name"] == item_name: - item["limits"]["enabled"] = True - found_item = item - break - if found_item is None: - raise RuntimeError(f"Item '{target_name} {packet_name} {item_name}' does not exist while enabling limits") - - TargetModel.set_packet(target_name, packet_name, packet, scope=scope) - - message = f"Enabling Limits For '{target_name} {packet_name} {item_name}'" - Logger.info(message, scope=scope) - - event = { - "type": "LIMITS_ENABLE_STATE", - "target_name": target_name, - "packet_name": packet_name, - "item_name": item_name, - "enabled": True, - "time_nsec": to_nsec_from_epoch(datetime.now(timezone.utc)), - "message": message, - } - LimitsEventTopic.write(event, scope=scope) + target_name, packet_names, item_name = _tlm_process_args_all_packets(args, "enable_limits", scope=scope) + for packet_name in packet_names: + authorize(permission="tlm_set", target_name=target_name, packet_name=packet_name, scope=scope) + for packet_name in packet_names: + _set_limits_enabled(target_name, packet_name, item_name, True, scope) # Disable limit checking for a telemetry item @@ -172,40 +146,14 @@ def enable_limits(*args, scope=OPENC3_SCOPE): # # Favor the first syntax where possible as it is more succinct. # -# @param args [String|Array] See the description for calling style +# @param args [String|Array] See the description for calling style. +# A packet name of LATEST applies to every packet which contains the item. def disable_limits(*args, scope=OPENC3_SCOPE): - target_name, packet_name, item_name = _tlm_process_args(args, "disable_limits", scope=scope) - authorize( - permission="tlm_set", - target_name=target_name, - packet_name=packet_name, - scope=scope, - ) - packet = TargetModel.packet(target_name, packet_name, scope=scope) - found_item = None - for item in packet["items"]: - if item["name"] == item_name: - item["limits"].pop("enabled", None) - found_item = item - break - if found_item is None: - raise RuntimeError(f"Item '{target_name} {packet_name} {item_name}' does not exist while disabling limits") - - TargetModel.set_packet(target_name, packet_name, packet, scope=scope) - - message = f"Disabling Limits for '{target_name} {packet_name} {item_name}'" - Logger.info(message, scope=scope) - - event = { - "type": "LIMITS_ENABLE_STATE", - "target_name": target_name, - "packet_name": packet_name, - "item_name": item_name, - "enabled": False, - "time_nsec": to_nsec_from_epoch(datetime.now(timezone.utc)), - "message": message, - } - LimitsEventTopic.write(event, scope=scope) + target_name, packet_names, item_name = _tlm_process_args_all_packets(args, "disable_limits", scope=scope) + for packet_name in packet_names: + authorize(permission="tlm_set", target_name=target_name, packet_name=packet_name, scope=scope) + for packet_name in packet_names: + _set_limits_enabled(target_name, packet_name, item_name, False, scope) # Get a Hash of all the limits sets defined for an item. Hash keys are the limit @@ -243,7 +191,8 @@ def get_limits(target_name, packet_name, item_name, scope=OPENC3_SCOPE): # Change the limits settings for a given item. By default, a new limits set called 'CUSTOM' -# is created to avoid overriding existing limits. +# is created to avoid overriding existing limits. A packet name of LATEST +# applies to every packet which contains the item. def set_limits( target_name, packet_name, @@ -259,73 +208,76 @@ def set_limits( enabled=True, scope=OPENC3_SCOPE, ): - authorize( - permission="tlm_set", - target_name=target_name, - packet_name=packet_name, - scope=scope, + target_name = target_name.upper() + packet_name = packet_name.upper() + item_name = item_name.upper() + packet_names = ( + _tlm_packet_names(target_name, packet_name, item_name, scope) if packet_name == "LATEST" else [packet_name] ) + for name in packet_names: + authorize(permission="tlm_set", target_name=target_name, packet_name=name, scope=scope) if (red_low > yellow_low) or (yellow_low >= yellow_high) or (yellow_high > red_high): raise RuntimeError("Invalid limits specified. Ensure yellow limits are within red limits.") if (green_low and green_high) and ( (yellow_low > green_low) or (green_low >= green_high) or (green_high > yellow_high) ): raise RuntimeError("Invalid limits specified. Ensure green limits are within yellow limits.") - packet = TargetModel.packet(target_name, packet_name, scope=scope) - found_item = None - for item in packet["items"]: - if item["name"] == item_name: - if item["limits"]: - if persistence: - item["limits"]["persistence_setting"] = persistence - if enabled: - item["limits"]["enabled"] = True - else: - item["limits"].pop("enabled", None) - limits = {} - limits["red_low"] = red_low - limits["yellow_low"] = yellow_low - limits["yellow_high"] = yellow_high - limits["red_high"] = red_high - if green_low and green_high: - limits["green_low"] = green_low - if green_low and green_high: - limits["green_high"] = green_high - item["limits"][limits_set] = limits - found_item = item - break - else: - raise RuntimeError("Cannot set_limits on item without any limits") - if found_item is None: - raise RuntimeError(f"Item '{target_name} {packet_name} {item_name}' does not exist while setting limits") - message = ( - f"Setting '{target_name} {packet_name} {item_name}' limits to {red_low} {yellow_low} {yellow_high} {red_high}" - ) - if green_low and green_high: - message += f" {green_low} {green_high}" - message += f" in set {limits_set} with persistence {persistence} as enabled {enabled}" - Logger.info(message, scope=scope) - - TargetModel.set_packet(target_name, packet_name, packet, scope=scope) - - event = { - "type": "LIMITS_SETTINGS", - "target_name": target_name, - "packet_name": packet_name, - "item_name": item_name, - "red_low": red_low, - "yellow_low": yellow_low, - "yellow_high": yellow_high, - "red_high": red_high, - "green_low": green_low, - "green_high": green_high, - "limits_set": limits_set, - "persistence": persistence, - "enabled": enabled, - "time_nsec": to_nsec_from_epoch(datetime.now(timezone.utc)), - "message": message, - } - LimitsEventTopic.write(event, scope=scope) + # Validate every packet before changing any so an error doesn't leave a partial update. + # TargetModel.packet returns a cached dict so it must not be modified until validated. + items = [] + for name in packet_names: + packet = TargetModel.packet(target_name, name, scope=scope) + item = next((packet_item for packet_item in packet["items"] if packet_item["name"] == item_name), None) + if item is None: + raise RuntimeError(f"Item '{target_name} {name} {item_name}' does not exist while setting limits") + if not item["limits"]: + raise RuntimeError("Cannot set_limits on item without any limits") + items.append((name, packet, item)) + for name, packet, item in items: + if persistence: + item["limits"]["persistence_setting"] = persistence + if enabled: + item["limits"]["enabled"] = True + else: + item["limits"].pop("enabled", None) + limits = {} + limits["red_low"] = red_low + limits["yellow_low"] = yellow_low + limits["yellow_high"] = yellow_high + limits["red_high"] = red_high + if green_low and green_high: + limits["green_low"] = green_low + limits["green_high"] = green_high + item["limits"][limits_set] = limits + + message = ( + f"Setting '{target_name} {name} {item_name}' limits to {red_low} {yellow_low} {yellow_high} {red_high}" + ) + if green_low and green_high: + message += f" {green_low} {green_high}" + message += f" in set {limits_set} with persistence {persistence} as enabled {enabled}" + Logger.info(message, scope=scope) + + TargetModel.set_packet(target_name, name, packet, scope=scope) + + event = { + "type": "LIMITS_SETTINGS", + "target_name": target_name, + "packet_name": name, + "item_name": item_name, + "red_low": red_low, + "yellow_low": yellow_low, + "yellow_high": yellow_high, + "red_high": red_high, + "green_low": green_low, + "green_high": green_high, + "limits_set": limits_set, + "persistence": persistence, + "enabled": enabled, + "time_nsec": to_nsec_from_epoch(datetime.now(timezone.utc)), + "message": message, + } + LimitsEventTopic.write(event, scope=scope) # Change the color associated with a telemetry item state in realtime. @@ -337,13 +289,16 @@ def set_limits( # @param item_name [str] Item name # @param state_name [str] Name of the state to change (e.g. 'CONNECTED') # @param color [str] New color for the state. Must be GREEN, YELLOW, or RED. +# A packet name of LATEST applies to every packet which contains the item. def set_state_color(target_name, packet_name, item_name, state_name, color, scope=OPENC3_SCOPE): - authorize( - permission="tlm_set", - target_name=target_name, - packet_name=packet_name, - scope=scope, + target_name = target_name.upper() + packet_name = packet_name.upper() + item_name = item_name.upper() + packet_names = ( + _tlm_packet_names(target_name, packet_name, item_name, scope) if packet_name == "LATEST" else [packet_name] ) + for name in packet_names: + authorize(permission="tlm_set", target_name=target_name, packet_name=name, scope=scope) state_name = str(state_name).upper() if color is not None: color = str(color).upper() @@ -351,46 +306,47 @@ def set_state_color(target_name, packet_name, item_name, state_name, color, scop raise RuntimeError( f"Invalid state color '{color}'. Must be one of {', '.join(PacketItem.VALID_STATE_COLORS)}." ) - packet = TargetModel.packet(target_name, packet_name, scope=scope) - found_item = None - for item in packet["items"]: - if item["name"] == item_name: - if not (item.get("states") and item["states"].get(state_name)): - raise RuntimeError( - f"State '{state_name}' does not exist for item '{target_name} {packet_name} {item_name}'" - ) - if color is None: - # Clear the state color - item["states"][state_name].pop("color", None) - else: - item["states"][state_name]["color"] = color - # Defining a state color implies limits are enabled for the item - if not item.get("limits"): - item["limits"] = {} - item["limits"]["enabled"] = True - found_item = item - break - if found_item is None: - raise RuntimeError(f"Item '{target_name} {packet_name} {item_name}' does not exist while setting state color") - if color is None: - message = f"Clearing '{target_name} {packet_name} {item_name}' state {state_name} color" - else: - message = f"Setting '{target_name} {packet_name} {item_name}' state {state_name} color to {color}" - Logger.info(message, scope=scope) - - TargetModel.set_packet(target_name, packet_name, packet, scope=scope) + # Validate every packet before changing any so an error doesn't leave a partial update. + # TargetModel.packet returns a cached dict so it must not be modified until validated. + items = [] + for name in packet_names: + packet = TargetModel.packet(target_name, name, scope=scope) + item = next((packet_item for packet_item in packet["items"] if packet_item["name"] == item_name), None) + if item is None: + raise RuntimeError(f"Item '{target_name} {name} {item_name}' does not exist while setting state color") + if not (item.get("states") and item["states"].get(state_name)): + raise RuntimeError(f"State '{state_name}' does not exist for item '{target_name} {name} {item_name}'") + items.append((name, packet, item)) + for name, packet, item in items: + if color is None: + # Clear the state color + item["states"][state_name].pop("color", None) + else: + item["states"][state_name]["color"] = color + # Defining a state color implies limits are enabled for the item + if not item.get("limits"): + item["limits"] = {} + item["limits"]["enabled"] = True - event = { - "type": "LIMITS_STATE_COLOR", - "target_name": target_name, - "packet_name": packet_name, - "item_name": item_name, - "state_name": state_name, - "color": color, - "time_nsec": to_nsec_from_epoch(datetime.now(timezone.utc)), - "message": message, - } - LimitsEventTopic.write(event, scope=scope) + if color is None: + message = f"Clearing '{target_name} {name} {item_name}' state {state_name} color" + else: + message = f"Setting '{target_name} {name} {item_name}' state {state_name} color to {color}" + Logger.info(message, scope=scope) + + TargetModel.set_packet(target_name, name, packet, scope=scope) + + event = { + "type": "LIMITS_STATE_COLOR", + "target_name": target_name, + "packet_name": name, + "item_name": item_name, + "state_name": state_name, + "color": color, + "time_nsec": to_nsec_from_epoch(datetime.now(timezone.utc)), + "message": message, + } + LimitsEventTopic.write(event, scope=scope) # Returns all limits_groups and their members @@ -535,3 +491,34 @@ def _limits_group(group_name, action, scope): last_packet_name = packet_name if last_target_name and last_packet_name: TargetModel.set_packet(last_target_name, last_packet_name, packet, scope=scope) + + +def _set_limits_enabled(target_name, packet_name, item_name, enabled, scope): + packet = TargetModel.packet(target_name, packet_name, scope=scope) + item = next((packet_item for packet_item in packet["items"] if packet_item["name"] == item_name), None) + if item is None: + action = "enabling" if enabled else "disabling" + raise RuntimeError(f"Item '{target_name} {packet_name} {item_name}' does not exist while {action} limits") + + if enabled: + item["limits"]["enabled"] = True + else: + item["limits"].pop("enabled", None) + TargetModel.set_packet(target_name, packet_name, packet, scope=scope) + + if enabled: + message = f"Enabling Limits For '{target_name} {packet_name} {item_name}'" + else: + message = f"Disabling Limits for '{target_name} {packet_name} {item_name}'" + Logger.info(message, scope=scope) + + event = { + "type": "LIMITS_ENABLE_STATE", + "target_name": target_name, + "packet_name": packet_name, + "item_name": item_name, + "enabled": enabled, + "time_nsec": to_nsec_from_epoch(datetime.now(timezone.utc)), + "message": message, + } + LimitsEventTopic.write(event, scope=scope) diff --git a/openc3/python/openc3/api/tlm_api.py b/openc3/python/openc3/api/tlm_api.py index 019c6a7377..dfbe46a02a 100644 --- a/openc3/python/openc3/api/tlm_api.py +++ b/openc3/python/openc3/api/tlm_api.py @@ -212,17 +212,15 @@ def inject_tlm( # # @param args The args must either be a string followed by a value or # three strings followed by a value (see the calling style in the -# description). +# description). A packet name of LATEST overrides the item in every +# packet which contains it. # @param type [Symbol] Telemetry type, :ALL (default), :RAW, :CONVERTED, :FORMATTED def override_tlm(*args, type="ALL", scope=OPENC3_SCOPE): - target_name, packet_name, item_name, value = _set_tlm_process_args(args, "override_tlm", scope=scope) - authorize( - permission="tlm_set", - target_name=target_name, - packet_name=packet_name, - scope=scope, - ) - CvtModel.override(target_name, packet_name, item_name, value, type=type, scope=scope) + target_name, packet_names, item_name, value = _set_tlm_process_args_all_packets(args, "override_tlm", scope=scope) + for packet_name in packet_names: + authorize(permission="tlm_set", target_name=target_name, packet_name=packet_name, scope=scope) + for packet_name in packet_names: + CvtModel.override(target_name, packet_name, item_name, value, type=type, scope=scope) # Get the list of CVT overrides @@ -241,18 +239,16 @@ def get_overrides(scope=OPENC3_SCOPE): # Favor the first syntax where possible as it is more succinct. # # @param args The args must either be a string or three strings -# (see the calling style in the description). +# (see the calling style in the description). A packet name of LATEST +# normalizes the item in every packet which contains it. # @param type [Symbol] Telemetry type, :ALL (default), :RAW, :CONVERTED, :FORMATTED # Also takes :ALL which means to normalize all telemetry types def normalize_tlm(*args, type="ALL", scope=OPENC3_SCOPE): - target_name, packet_name, item_name = _tlm_process_args(args, "normalize_tlm", scope=scope) - authorize( - permission="tlm_set", - target_name=target_name, - packet_name=packet_name, - scope=scope, - ) - CvtModel.normalize(target_name, packet_name, item_name, type=type, scope=scope) + target_name, packet_names, item_name = _tlm_process_args_all_packets(args, "normalize_tlm", scope=scope) + for packet_name in packet_names: + authorize(permission="tlm_set", target_name=target_name, packet_name=packet_name, scope=scope) + for packet_name in packet_names: + CvtModel.normalize(target_name, packet_name, item_name, type=type, scope=scope) # Returns the raw buffer for a telemetry packet. @@ -722,6 +718,31 @@ def _validate_tlm_type(tlm_type): def _tlm_process_args(args, method_name, cache_timeout=0.1, scope=OPENC3_SCOPE): + target_name, packet_name, item_name = _tlm_extract_args(args, method_name) + packet_name = _tlm_resolve_packet_name(target_name, packet_name, item_name, cache_timeout, scope) + return target_name, packet_name, item_name + + +def _set_tlm_process_args(args, method_name, cache_timeout=0.1, scope=OPENC3_SCOPE): + target_name, packet_name, item_name, value = _set_tlm_extract_args(args, method_name) + packet_name = _tlm_resolve_packet_name(target_name, packet_name, item_name, cache_timeout, scope) + return target_name, packet_name, item_name, value + + +# Like _tlm_process_args but returns a list of packet names where LATEST +# expands to every packet containing the item. Used by APIs which change +# an item so the change applies wherever the item appears. +def _tlm_process_args_all_packets(args, method_name, scope=OPENC3_SCOPE): + target_name, packet_name, item_name = _tlm_extract_args(args, method_name) + return target_name, _tlm_packet_names(target_name, packet_name, item_name, scope), item_name + + +def _set_tlm_process_args_all_packets(args, method_name, scope=OPENC3_SCOPE): + target_name, packet_name, item_name, value = _set_tlm_extract_args(args, method_name) + return target_name, _tlm_packet_names(target_name, packet_name, item_name, scope), item_name, value + + +def _tlm_extract_args(args, method_name): match len(args): case 1: target_name, packet_name, item_name = extract_fields_from_tlm_text(args[0]) @@ -732,18 +753,10 @@ def _tlm_process_args(args, method_name, cache_timeout=0.1, scope=OPENC3_SCOPE): case _: # Invalid number of arguments raise RuntimeError(f"ERROR: Invalid number of arguments ({len(args)}) passed to {method_name}()") - target_name = target_name.upper() - packet_name = packet_name.upper() - item_name = item_name.upper() - if packet_name == "LATEST": - packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout, scope) - else: - # Determine if this item exists, it will raise appropriate errors if not - TargetModel.packet_item(target_name, packet_name, item_name, scope=scope) - return target_name, packet_name, item_name + return target_name.upper(), packet_name.upper(), item_name.upper() -def _set_tlm_process_args(args, method_name, cache_timeout=0.1, scope=OPENC3_SCOPE): +def _set_tlm_extract_args(args, method_name): match len(args): case 1: target_name, packet_name, item_name, value = extract_fields_from_set_tlm_text(args[0]) @@ -755,13 +768,25 @@ def _set_tlm_process_args(args, method_name, cache_timeout=0.1, scope=OPENC3_SCO case _: # Invalid number of arguments raise RuntimeError(f"ERROR: Invalid number of arguments ({len(args)}) passed to {method_name}()") - target_name = target_name.upper() - packet_name = packet_name.upper() - item_name = item_name.upper() + return target_name.upper(), packet_name.upper(), item_name.upper(), value + + +# Resolve LATEST to the most recently received packet containing the item +def _tlm_resolve_packet_name(target_name, packet_name, item_name, cache_timeout=0.1, scope=OPENC3_SCOPE): if packet_name == "LATEST": - packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout, scope) - else: - # Determine if this item exists, it will raise appropriate errors if not - TargetModel.packet_item(target_name, packet_name, item_name, scope=scope) + return CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout, scope) + # Determine if this item exists, it will raise appropriate errors if not + TargetModel.packet_item(target_name, packet_name, item_name, scope=scope) + return packet_name - return target_name, packet_name, item_name, value + +# Expand LATEST to every packet containing the item +def _tlm_packet_names(target_name, packet_name, item_name, scope=OPENC3_SCOPE): + if packet_name == "LATEST": + packet_names = TargetModel.get_item_to_packet_map(target_name, scope=scope).get(item_name) + if packet_names is None: + raise RuntimeError(f"Item '{target_name} LATEST {item_name}' does not exist for scope: {scope}") + return packet_names + # Determine if this item exists, it will raise appropriate errors if not + TargetModel.packet_item(target_name, packet_name, item_name, scope=scope) + return [packet_name] diff --git a/openc3/python/test/api/test_limits_api.py b/openc3/python/test/api/test_limits_api.py index 749a6c9a5d..238731ca29 100644 --- a/openc3/python/test/api/test_limits_api.py +++ b/openc3/python/test/api/test_limits_api.py @@ -278,6 +278,37 @@ def test_set_state_color_writes_a_limits_state_color_event_with_none_when_cleari self.assertEqual(event["state_name"], "UNAVAILABLE") self.assertIsNone(event["color"]) + def test_enables_and_disables_latest_limits_in_every_packet_which_contains_the_item(self): + packet_names = TargetModel.get_item_to_packet_map("INST", scope="DEFAULT")["CCSDSSEQCNT"] + self.assertGreater(len(packet_names), 1) + disable_limits("INST LATEST CCSDSSEQCNT") + for packet_name in packet_names: + self.assertFalse(limits_enabled("INST", packet_name, "CCSDSSEQCNT")) + enable_limits("INST", "LATEST", "CCSDSSEQCNT") + for packet_name in packet_names: + self.assertTrue(limits_enabled("INST", packet_name, "CCSDSSEQCNT")) + + def test_set_limits_through_latest(self): + set_limits("INST", "LATEST", "TEMP1", 0.0, 10.0, 20.0, 30.0) + self.assertEqual(get_limits("INST", "HEALTH_STATUS", "TEMP1")["CUSTOM"], [0.0, 10.0, 20.0, 30.0]) + + def test_set_limits_through_latest_sets_no_packet_if_any_packet_fails(self): + item_map = {"TEMP1": ["HEALTH_STATUS", "MECH"]} + with ( + patch.object(TargetModel, "get_item_to_packet_map", return_value=item_map), + self.assertRaisesRegex(RuntimeError, "Item 'INST MECH TEMP1' does not exist while setting limits"), + ): + set_limits("INST", "LATEST", "TEMP1", 0.0, 10.0, 20.0, 30.0) + self.assertNotIn("CUSTOM", get_limits("INST", "HEALTH_STATUS", "TEMP1")) + + def test_set_state_color_through_latest_in_every_packet_which_contains_the_item(self): + packet_names = TargetModel.get_item_to_packet_map("INST", scope="DEFAULT")["CCSDSTYPE"] + self.assertGreater(len(packet_names), 1) + set_state_color("INST", "LATEST", "CCSDSTYPE", "TLM", "GREEN") + for packet_name in packet_names: + item = TargetModel.packet_item("INST", packet_name, "CCSDSTYPE", scope="DEFAULT") + self.assertEqual(item["states"]["TLM"]["color"], "GREEN") + def test_get_limits_groups_returns_all_the_limits_groups(self): self.assertEqual( get_limits_groups(), diff --git a/openc3/python/test/api/test_tlm_api.py b/openc3/python/test/api/test_tlm_api.py index d349d6def1..c3d6a464c6 100644 --- a/openc3/python/test/api/test_tlm_api.py +++ b/openc3/python/test/api/test_tlm_api.py @@ -332,6 +332,16 @@ def test_overrides_complains_about_unknown_targets_packets_and_parameters(self): with self.assertRaisesRegex(RuntimeError, "does not exist"): override_tlm("INST", "HEALTH_STATUS", "BLAH", 1) + def test_overrides_and_normalizes_latest_in_every_packet_which_contains_the_item(self): + packet_names = TargetModel.get_item_to_packet_map("INST", scope="DEFAULT")["CCSDSSEQCNT"] + self.assertGreater(len(packet_names), 1) + override_tlm("INST LATEST CCSDSSEQCNT = 5", type="RAW") + self.assertCountEqual([o["packet_name"] for o in get_overrides()], packet_names) + for packet_name in packet_names: + self.assertEqual(tlm("INST", packet_name, "CCSDSSEQCNT", type="RAW"), 5) + normalize_tlm("INST LATEST CCSDSSEQCNT") + self.assertEqual(get_overrides(), []) + def test_overrides_complains_with_too_many_parameters(self): with self.assertRaisesRegex(RuntimeError, "Invalid number of arguments"): override_tlm("INST", "HEALTH_STATUS", "TEMP1", "TEMP2", 0.0) diff --git a/openc3/spec/api/limits_api_spec.rb b/openc3/spec/api/limits_api_spec.rb index 285617109a..01a50e88de 100644 --- a/openc3/spec/api/limits_api_spec.rb +++ b/openc3/spec/api/limits_api_spec.rb @@ -233,6 +233,43 @@ def with_decom_microservice end end + describe "LATEST limits changes" do + it "enables and disables limits in every packet which contains the item" do + packet_names = TargetModel.get_item_to_packet_map('INST', scope: 'DEFAULT')['CCSDSSEQCNT'] + expect(packet_names.length).to be > 1 + @api.disable_limits("INST LATEST CCSDSSEQCNT") + packet_names.each do |packet_name| + expect(@api.limits_enabled?("INST", packet_name, "CCSDSSEQCNT")).to be false + end + @api.enable_limits("INST", "LATEST", "CCSDSSEQCNT") + packet_names.each do |packet_name| + expect(@api.limits_enabled?("INST", packet_name, "CCSDSSEQCNT")).to be true + end + end + + it "sets limits through LATEST" do + @api.set_limits("INST", "LATEST", "TEMP1", 0.0, 10.0, 20.0, 30.0) + expect(@api.get_limits("INST", "HEALTH_STATUS", "TEMP1")['CUSTOM']).to eql([0.0, 10.0, 20.0, 30.0]) + end + + it "sets limits in no packet if any packet fails" do + allow(TargetModel).to receive(:get_item_to_packet_map).and_return({ 'TEMP1' => ['HEALTH_STATUS', 'MECH'] }) + expect { @api.set_limits("INST", "LATEST", "TEMP1", 0.0, 10.0, 20.0, 30.0) }.to \ + raise_error(RuntimeError, "Item 'INST MECH TEMP1' does not exist while setting limits") + expect(@api.get_limits("INST", "HEALTH_STATUS", "TEMP1")['CUSTOM']).to be_nil + end + + it "sets the state color in every packet which contains the item" do + packet_names = TargetModel.get_item_to_packet_map('INST', scope: 'DEFAULT')['CCSDSTYPE'] + expect(packet_names.length).to be > 1 + @api.set_state_color("INST", "LATEST", "CCSDSTYPE", "TLM", "GREEN") + packet_names.each do |packet_name| + item = TargetModel.packet_item("INST", packet_name, "CCSDSTYPE", scope: "DEFAULT") + expect(item['states']['TLM']['color']).to eql('GREEN') + end + end + end + describe "get_limits_groups" do it "returns an empty hash with no groups" do # Remove all limits_groups diff --git a/openc3/spec/api/tlm_api_spec.rb b/openc3/spec/api/tlm_api_spec.rb index ab7db8f41f..4a199d95f2 100644 --- a/openc3/spec/api/tlm_api_spec.rb +++ b/openc3/spec/api/tlm_api_spec.rb @@ -392,6 +392,20 @@ def test_tlm_unknown(method) end end + describe "override_tlm and normalize_tlm with LATEST" do + it "overrides and normalizes the item in every packet which contains it" do + packet_names = TargetModel.get_item_to_packet_map('INST', scope: 'DEFAULT')['CCSDSSEQCNT'] + expect(packet_names.length).to be > 1 + @api.override_tlm("INST LATEST CCSDSSEQCNT = 5", type: :RAW) + expect(@api.get_overrides().map { |o| o['packet_name'] }).to match_array(packet_names) + packet_names.each do |packet_name| + expect(@api.tlm("INST", packet_name, "CCSDSSEQCNT", type: :RAW)).to eql(5) + end + @api.normalize_tlm("INST LATEST CCSDSSEQCNT") + expect(@api.get_overrides()).to eql([]) + end + end + describe "get_overrides" do it "returns empty array with no overrides" do expect(@api.get_overrides()).to eql([]) From e4bf5f51b3261921bea3e1af1520b692dc63edc7 Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Mon, 12 Oct 2026 17:08:32 -0600 Subject: [PATCH 04/14] feat(telemetry): resolve LATEST at the requested time in history - get_tlm_values with start_time queries every packet containing a LATEST item instead of the packet received most recently now - A point in time queries each candidate packet on its own since the ASOF JOIN only looks up other packets at the first packet's last row - A range puts regular items first so they drive the rows; a range of only LATEST items keeps using the packet received most recently now - Alias every tsdb_lookup column since QuestDB renames duplicate names across tables (VALUE_2), which broke decoding of the second one - Add a flatten option to tsdb_lookup and QuestDB integration specs Co-Authored-By: Claude Opus 5.5 --- openc3/lib/openc3/api/tlm_api.rb | 99 +++++++++++++- openc3/lib/openc3/models/cvt_model.rb | 4 +- openc3/lib/openc3/utilities/questdb_client.rb | 30 +++-- openc3/python/openc3/api/tlm_api.py | 121 +++++++++++++++++- openc3/python/openc3/models/cvt_model.py | 3 +- .../python/openc3/utilities/questdb_client.py | 35 +++-- openc3/python/test/api/test_tlm_api.py | 93 ++++++++++++++ .../test/utilities/test_questdb_client.py | 19 +++ openc3/spec/api/tlm_api_spec.rb | 72 +++++++++++ openc3/spec/utilities/questdb_client_spec.rb | 19 ++- .../tsdb/python/test_tlm_api_latest_tsdb.py | 115 +++++++++++++++++ .../tsdb/ruby/tlm_api_latest_tsdb_spec.rb | 93 ++++++++++++++ 12 files changed, 678 insertions(+), 25 deletions(-) create mode 100644 openc3/test/integration/tsdb/python/test_tlm_api_latest_tsdb.py create mode 100644 openc3/test/integration/tsdb/ruby/tlm_api_latest_tsdb_spec.rb diff --git a/openc3/lib/openc3/api/tlm_api.rb b/openc3/lib/openc3/api/tlm_api.rb index 85095274d1..415239b4de 100644 --- a/openc3/lib/openc3/api/tlm_api.rb +++ b/openc3/lib/openc3/api/tlm_api.rb @@ -351,7 +351,15 @@ def get_tlm_values(items, stale_time: 30, cache_timeout: nil, manual: false, sta parts = item.to_s.upcase.split('__') raise ArgumentError, "items must be formatted as TGT__PKT__ITEM__TYPE" if parts.length < 4 || parts.length > 5 target_name, packet_name, item_name, value_type, limits = parts - if packet_name == 'LATEST' # Lookup packet_name in case of LATEST + if packet_name == 'LATEST' + if start_time + # The packet received most recently now isn't necessarily the one received most + # recently at start_time so keep every candidate packet for _tlm_latest_history + candidates = _tlm_packet_names(target_name, packet_name, item_name, scope: scope) + candidates.each { |candidate| packets << [target_name, candidate] } + cvt_items << [target_name, packet_name, item_name, value_type, limits, candidates] + next + end packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout: cache_timeout, scope: scope) end # NOTE: limits is required by the historical (start_time) QuestDB lookup @@ -362,6 +370,9 @@ def get_tlm_values(items, stale_time: 30, cache_timeout: nil, manual: false, sta packets.each do |target_name, packet_name| authorize(permission: 'tlm', target_name: target_name, packet_name: packet_name, manual: manual, scope: scope, token: token) end + if cvt_items.any? { |cvt_item| cvt_item.length == 6 } + return _tlm_latest_history(cvt_items, start_time: start_time, end_time: end_time, cache_timeout: cache_timeout, scope: scope) + end CvtModel.get_tlm_values(cvt_items, stale_time: stale_time, cache_timeout: cache_timeout, start_time: start_time, end_time: end_time, scope: scope) end @@ -669,6 +680,92 @@ def _tlm_resolve_packet_name(target_name, packet_name, item_name, cache_timeout: return packet_name end + # Historical lookup where some items are LATEST. Those items have a 6th element + # holding every packet which contains the item. + def _tlm_latest_history(cvt_items, start_time:, end_time:, cache_timeout:, scope:) + latest = cvt_items.each_index.select { |index| cvt_items[index].length == 6 } + regular = cvt_items.each_index.reject { |index| cvt_items[index].length == 6 } + + if end_time + # The QuestDB lookup ASOF JOINs every packet to the rows of the first, so + # a range needs a regular item first to drive the rows. Without one keep + # the realtime behavior of using the packet received most recently now. + if regular.all? { |index| cvt_items[index][2].nil? } + cvt_items = cvt_items.map do |target_name, packet_name, item_name, value_type, limits, _| + packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout: cache_timeout, scope: scope) if packet_name == 'LATEST' + [target_name, packet_name, item_name, value_type, limits] + end + return CvtModel.tsdb_lookup(cvt_items, start_time: start_time, end_time: end_time, scope: scope) + end + + query = [] + query_index = {} + add = lambda { |query_item| query_index[query_item] ||= (query << query_item; query.length - 1) } + positions = Array.new(cvt_items.length) + regular.each { |index| positions[index] = add.call(cvt_items[index]) } + latest.each do |index| + target_name, _, item_name, value_type, limits, candidates = cvt_items[index] + positions[index] = candidates.map do |candidate| + [add.call([target_name, candidate, item_name, value_type, limits]), + add.call([target_name, candidate, 'PACKET_TIMESECONDS', 'RAW', nil])] + end + end + rows = CvtModel.tsdb_lookup(query, start_time: start_time, end_time: end_time, scope: scope, flatten: false) + return rows unless rows.is_a?(Array) and rows.length > 0 + rows = rows.map do |row| + positions.map do |position| + next row[position] if position.is_a?(Integer) + # Take the candidate with the newest packet time at this row + _tlm_newest(position.map { |value_index, time_index| [row[time_index][0], row[value_index]] }) || [nil, nil] + end + end + # Match tsdb_lookup which returns a flat array for a single row + return rows.length == 1 ? rows[0] : rows + end + + # A point in time returns the last row of the first packet before start_time + # with the other packets ASOF JOINed at that row. A candidate packet received + # after that row would be missed so query each candidate packet on its own. + found = false + row = Array.new(cvt_items.length) { [nil, nil] } + unless regular.all? { |index| cvt_items[index][2].nil? } + result = CvtModel.tsdb_lookup(regular.map { |index| cvt_items[index] }, start_time: start_time, scope: scope, flatten: false) + if result.is_a?(Array) and result.length > 0 + found = true + regular.each_with_index { |index, result_index| row[index] = result[0][result_index] } + end + end + # Group the LATEST items by candidate packet so each packet is queried once + by_packet = {} + latest.each do |index| + target_name, _, item_name, value_type, limits, candidates = cvt_items[index] + candidates.each do |candidate| + (by_packet[[target_name, candidate]] ||= {})[index] = [target_name, candidate, item_name, value_type, limits] + end + end + found_values = Hash.new { |hash, key| hash[key] = [] } + by_packet.each do |(target_name, packet_name), packet_items| + query = packet_items.values.uniq + query << [target_name, packet_name, 'PACKET_TIMESECONDS', 'RAW', nil] + result = CvtModel.tsdb_lookup(query, start_time: start_time, scope: scope, flatten: false) + next unless result.is_a?(Array) and result.length > 0 + values = result[0] + packet_items.each { |index, query_item| found_values[index] << [values[-1][0], values[query.index(query_item)]] } + end + found_values.each do |index, candidates| + value = _tlm_newest(candidates) + next unless value + found = true + row[index] = value + end + return found ? row : {} + end + + # Return the value with the newest time from [time, value] pairs, or nil + def _tlm_newest(candidates) + candidates.select { |time, _| time }.max_by { |time, _| time }&.last + end + # Expand LATEST to every packet containing the item def _tlm_packet_names(target_name, packet_name, item_name, scope: $openc3_scope) if packet_name == 'LATEST' diff --git a/openc3/lib/openc3/models/cvt_model.rb b/openc3/lib/openc3/models/cvt_model.rb index 2166c3cad4..486b879dbd 100644 --- a/openc3/lib/openc3/models/cvt_model.rb +++ b/openc3/lib/openc3/models/cvt_model.rb @@ -134,8 +134,8 @@ def self.get_item(target_name, packet_name, item_name, type:, cache_timeout: nil end end - def self.tsdb_lookup(items, start_time:, end_time: nil, scope: $openc3_scope) - QuestDBClient.tsdb_lookup(items, start_time: start_time, end_time: end_time, scope: scope) + def self.tsdb_lookup(items, start_time:, end_time: nil, scope: $openc3_scope, flatten: true) + QuestDBClient.tsdb_lookup(items, start_time: start_time, end_time: end_time, scope: scope, flatten: flatten) end # Return all item values and limit state from the CVT diff --git a/openc3/lib/openc3/utilities/questdb_client.rb b/openc3/lib/openc3/utilities/questdb_client.rb index dcb8a7bb7c..daec16a6b6 100644 --- a/openc3/lib/openc3/utilities/questdb_client.rb +++ b/openc3/lib/openc3/utilities/questdb_client.rb @@ -925,12 +925,16 @@ def self.decode_reduced_row(row) # @param start_time [String, Numeric] Start timestamp for the query # @param end_time [String, Numeric, nil] End timestamp, or nil for "latest single row" # @param scope [String] Scope name + # @param flatten [Boolean] Whether a single row result is returned as a flat array # @return [Array, Hash] Array of [value, limits_state] pairs per row, or {} if no results. - # Single-row results return a flat array; multi-row results return array of arrays. - def self.tsdb_lookup(items, start_time:, end_time: nil, scope: "DEFAULT") + # Single-row results return a flat array (unless flatten is false); multi-row results return array of arrays. + def self.tsdb_lookup(items, start_time:, end_time: nil, scope: "DEFAULT", flatten: true) # Every item is a placeholder for an item which doesn't exist, so there's # nothing to query. Return a single row of nil values, one per item. - return Array.new(items.length) { [nil, nil] } if items.all? { |item| item[2].nil? } + if items.all? { |item| item[2].nil? } + row = Array.new(items.length) { [nil, nil] } + return flatten ? row : [row] + end # Group items by db_shard number while preserving their original positions db_shard_groups = {} # db_shard => { positions: [], items: [] } @@ -945,7 +949,7 @@ def self.tsdb_lookup(items, start_time:, end_time: nil, scope: "DEFAULT") # Single-db_shard fast path (most common case) if db_shard_groups.length == 1 db_shard, group = db_shard_groups.first - return tsdb_lookup_single_db_shard(group[:items], start_time: start_time, end_time: end_time, scope: scope, db_shard: db_shard) + return tsdb_lookup_single_db_shard(group[:items], start_time: start_time, end_time: end_time, scope: scope, db_shard: db_shard, flatten: flatten) end # Cross-db_shard: execute per-db_shard queries (as arrays of rows) and merge results @@ -970,7 +974,7 @@ def self.tsdb_lookup(items, start_time:, end_time: nil, scope: "DEFAULT") end end # Match the single db_shard behavior of returning a flat array for a single row - return merged[0] if max_rows == 1 + return merged[0] if flatten and max_rows == 1 merged end @@ -986,6 +990,14 @@ def self.tsdb_lookup_single_db_shard(items, start_time:, end_time: nil, scope: " calculated_items = {} needed_timestamps = {} current_position = 0 + # Columns with the same name in different tables come back renamed by QuestDB + # (e.g. VALUE and VALUE_2) so alias every column and map it back when decoding + aliases = {} + add_column = lambda do |col_name| + alias_name = "__col#{aliases.length}" + aliases[alias_name] = col_name + names << "\"#{col_name}\" as #{alias_name}" + end items.each do |item| target_name, packet_name, orig_item_name, value_type, limits = item @@ -1000,7 +1012,7 @@ def self.tsdb_lookup_single_db_shard(items, start_time:, end_time: nil, scope: " index = tables.find_index {|k,_v| k == table_name } if STORED_TIMESTAMP_ITEMS.include?(orig_item_name) - names << "\"T#{index}.#{orig_item_name}\"" + add_column.call("T#{index}.#{orig_item_name}") current_position += 1 next end @@ -1030,11 +1042,11 @@ def self.tsdb_lookup_single_db_shard(items, start_time:, end_time: nil, scope: " suffix = column_suffix_for_value_type(value_type) col_name = "T#{index}.#{safe_item_name}#{suffix}" - names << "\"#{col_name}\"" + add_column.call(col_name) item_types[col_name] = resolve_item_type(item_def, value_type) current_position += 1 if limits - names << "\"T#{index}.#{safe_item_name}__L\"" + add_column.call("T#{index}.#{safe_item_name}__L") end end @@ -1080,7 +1092,7 @@ def self.tsdb_lookup_single_db_shard(items, start_time:, end_time: nil, scope: " row_index = 0 row_timestamps = {} tuples.each do |tuple| - col_name = tuple[0] + col_name = aliases.fetch(tuple[0], tuple[0]) col_value = tuple[1] if col_name.include?("__L") data[row_num][row_index - 1][1] = col_value diff --git a/openc3/python/openc3/api/tlm_api.py b/openc3/python/openc3/api/tlm_api.py index dfbe46a02a..94c8d585ad 100644 --- a/openc3/python/openc3/api/tlm_api.py +++ b/openc3/python/openc3/api/tlm_api.py @@ -394,6 +394,8 @@ def get_tlm_values( raise TypeError("items must be array of strings: ['TGT__PKT__ITEM__TYPE', ...]") packets = [] cvt_items = [] + # Index of each historical LATEST item in cvt_items => every packet containing the item + latest_candidates = {} for item in items: # get_tlm_available returns None for items which don't exist and its result is # passed directly here, so None is a placeholder which returns a None value @@ -410,8 +412,15 @@ def get_tlm_values( target_name, packet_name, item_name, value_type = parts[0:4] limits = parts[4] if len(parts) == 5 else None if packet_name == "LATEST": + if start_time is not None: + # The packet received most recently now isn't necessarily the one received most + # recently at start_time so keep every candidate packet for _tlm_latest_history + candidates = _tlm_packet_names(target_name, packet_name, item_name, scope) + packets.extend([target_name, candidate] for candidate in candidates) + latest_candidates[len(cvt_items)] = candidates + cvt_items.append(TlmItem(target_name, packet_name, item_name, value_type, limits)) + continue packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout, scope) - # Change packet_name in case of LATEST and ensure upcase # NOTE: limits is required by the historical (start_time) QuestDB lookup cvt_items.append(TlmItem(target_name, packet_name, item_name, value_type, limits)) packets.append([target_name, packet_name]) @@ -424,6 +433,8 @@ def get_tlm_values( packet_name=name[1], scope=scope, ) + if latest_candidates: + return _tlm_latest_history(cvt_items, latest_candidates, start_time, end_time, cache_timeout, scope) return CvtModel.get_tlm_values(cvt_items, stale_time, cache_timeout, start_time, end_time, scope) @@ -780,6 +791,114 @@ def _tlm_resolve_packet_name(target_name, packet_name, item_name, cache_timeout= return packet_name +# Historical lookup where some items are LATEST. latest_candidates maps the index +# of each LATEST item in cvt_items to every packet which contains the item. +def _tlm_latest_history(cvt_items, latest_candidates, start_time, end_time, cache_timeout, scope): + regular = [index for index in range(len(cvt_items)) if index not in latest_candidates] + + if end_time is not None: + # The QuestDB lookup ASOF JOINs every packet to the rows of the first, so + # a range needs a regular item first to drive the rows. Without one keep + # the realtime behavior of using the packet received most recently now. + if all(cvt_items[index].item_name is None for index in regular): + resolved = [] + for cvt_item in cvt_items: + if cvt_item.packet_name == "LATEST": + packet_name = CvtModel.determine_latest_packet_for_item( + cvt_item.target_name, cvt_item.item_name, cache_timeout, scope + ) + cvt_item = cvt_item._replace(packet_name=packet_name) + resolved.append(cvt_item) + return CvtModel.tsdb_lookup(resolved, start_time=start_time, end_time=end_time, scope=scope) + + query = [] + query_index = {} + + def add(query_item): + if query_item not in query_index: + query_index[query_item] = len(query) + query.append(query_item) + return query_index[query_item] + + positions = [None] * len(cvt_items) + for index in regular: + positions[index] = add(cvt_items[index]) + for index, candidates in latest_candidates.items(): + cvt_item = cvt_items[index] + positions[index] = [ + ( + add(cvt_item._replace(packet_name=candidate)), + add(TlmItem(cvt_item.target_name, candidate, "PACKET_TIMESECONDS", "RAW", None)), + ) + for candidate in candidates + ] + rows = CvtModel.tsdb_lookup(query, start_time=start_time, end_time=end_time, scope=scope, flatten=False) + if not isinstance(rows, list) or len(rows) == 0: + return rows + collapsed = [] + for row in rows: + values = [] + for position in positions: + if isinstance(position, int): + values.append(row[position]) + else: + # Take the candidate with the newest packet time at this row + newest = _tlm_newest( + [(row[time_index][0], row[value_index]) for value_index, time_index in position] + ) + values.append(newest or [None, None]) + collapsed.append(values) + # Match tsdb_lookup which returns a flat array for a single row + return collapsed[0] if len(collapsed) == 1 else collapsed + + # A point in time returns the last row of the first packet before start_time + # with the other packets ASOF JOINed at that row. A candidate packet received + # after that row would be missed so query each candidate packet on its own. + found = False + row = [[None, None] for _ in cvt_items] + if not all(cvt_items[index].item_name is None for index in regular): + result = CvtModel.tsdb_lookup( + [cvt_items[index] for index in regular], start_time=start_time, scope=scope, flatten=False + ) + if isinstance(result, list) and len(result) > 0: + found = True + for result_index, index in enumerate(regular): + row[index] = result[0][result_index] + # Group the LATEST items by candidate packet so each packet is queried once + by_packet = {} + for index, candidates in latest_candidates.items(): + cvt_item = cvt_items[index] + for candidate in candidates: + by_packet.setdefault((cvt_item.target_name, candidate), {})[index] = cvt_item._replace( + packet_name=candidate + ) + found_values = {} + for (target_name, packet_name), packet_items in by_packet.items(): + query = list(dict.fromkeys(packet_items.values())) + query.append(TlmItem(target_name, packet_name, "PACKET_TIMESECONDS", "RAW", None)) + result = CvtModel.tsdb_lookup(query, start_time=start_time, scope=scope, flatten=False) + if not isinstance(result, list) or len(result) == 0: + continue + values = result[0] + for index, query_item in packet_items.items(): + found_values.setdefault(index, []).append((values[-1][0], values[query.index(query_item)])) + for index, candidates in found_values.items(): + value = _tlm_newest(candidates) + if value is None: + continue + found = True + row[index] = value + return row if found else {} + + +# Return the value with the newest time from (time, value) pairs, or None +def _tlm_newest(candidates): + candidates = [candidate for candidate in candidates if candidate[0] is not None] + if not candidates: + return None + return max(candidates, key=lambda candidate: candidate[0])[1] + + # Expand LATEST to every packet containing the item def _tlm_packet_names(target_name, packet_name, item_name, scope=OPENC3_SCOPE): if packet_name == "LATEST": diff --git a/openc3/python/openc3/models/cvt_model.py b/openc3/python/openc3/models/cvt_model.py index 5a63b466f3..46b529b93e 100644 --- a/openc3/python/openc3/models/cvt_model.py +++ b/openc3/python/openc3/models/cvt_model.py @@ -192,9 +192,10 @@ def tsdb_lookup( start_time: str, end_time: str | None = None, scope: str = OPENC3_SCOPE, + flatten: bool = True, ): """Query historical telemetry data from TSDB""" - return QuestDBClient.tsdb_lookup(items, start_time=start_time, end_time=end_time, scope=scope) + return QuestDBClient.tsdb_lookup(items, start_time=start_time, end_time=end_time, scope=scope, flatten=flatten) # Return all item values and limit state from the CVT # diff --git a/openc3/python/openc3/utilities/questdb_client.py b/openc3/python/openc3/utilities/questdb_client.py index f0c6f2d075..0652e1e736 100644 --- a/openc3/python/openc3/utilities/questdb_client.py +++ b/openc3/python/openc3/utilities/questdb_client.py @@ -781,7 +781,7 @@ def db_shard_for_target(cls, target_name, scope="DEFAULT"): return Store.db_shard_for_target(target_name, scope=scope) @classmethod - def tsdb_lookup(cls, items, start_time, end_time=None, scope="DEFAULT"): + def tsdb_lookup(cls, items, start_time, end_time=None, scope="DEFAULT", flatten=True): """Query historical telemetry data from QuestDB for a list of items. Builds the SQL query, executes it, and decodes all results. Supports cross-db_shard queries by grouping items by db_shard, executing @@ -793,17 +793,20 @@ def tsdb_lookup(cls, items, start_time, end_time=None, scope="DEFAULT"): start_time: Start timestamp for the query end_time: End timestamp, or None for "latest single row" scope: Scope name + flatten: Whether a single row result is returned as a flat array Returns: Array of [value, limits_state] pairs per row, or {} if no results. - Single-row results return a flat array; multi-row results return array of arrays. + Single-row results return a flat array (unless flatten is False); multi-row results + return array of arrays. """ items = [TlmItem(*item) for item in items] # Every item is a placeholder for an item which doesn't exist, so there's # nothing to query. Return a single row of None values, one per item. if all(item.item_name is None for item in items): - return [[None, None] for _ in items] + row = [[None, None] for _ in items] + return row if flatten else [row] # Group items by db_shard number while preserving their original positions db_shard_groups = {} # db_shard => {"positions": [], "items": []} @@ -817,7 +820,12 @@ def tsdb_lookup(cls, items, start_time, end_time=None, scope="DEFAULT"): if len(db_shard_groups) == 1: db_shard, group = next(iter(db_shard_groups.items())) return cls._tsdb_lookup_single_db_shard( - group["items"], start_time=start_time, end_time=end_time, scope=scope, db_shard=db_shard + group["items"], + start_time=start_time, + end_time=end_time, + scope=scope, + db_shard=db_shard, + flatten=flatten, ) # Cross-db_shard: execute per-db_shard queries (as lists of rows) and merge results @@ -842,7 +850,7 @@ def tsdb_lookup(cls, items, start_time, end_time=None, scope="DEFAULT"): if db_shard_idx < len(row) and row[db_shard_idx]: merged[row_num][orig_pos] = row[db_shard_idx] # Match the single db_shard behavior of returning a flat array for a single row - if max_rows == 1: + if flatten and max_rows == 1: return merged[0] return merged @@ -860,6 +868,14 @@ def _tsdb_lookup_single_db_shard(cls, items, start_time, end_time=None, scope="D calculated_items = {} needed_timestamps = {} current_position = 0 + # Columns with the same name in different tables come back renamed by QuestDB + # (e.g. VALUE and VALUE_2) so alias every column and map it back when decoding + aliases = {} + + def add_column(col_name): + alias_name = f"__col{len(aliases)}" + aliases[alias_name] = col_name + names.append(f'"{col_name}" as {alias_name}') for item in items: target_name, packet_name, item_name, value_type, limits = item @@ -874,7 +890,7 @@ def _tsdb_lookup_single_db_shard(cls, items, start_time, end_time=None, scope="D index = list(tables.keys()).index(table_name) if item_name in cls.STORED_TIMESTAMP_ITEMS: - names.append(f'"T{index}.{item_name}"') + add_column(f"T{index}.{item_name}") current_position += 1 continue @@ -902,12 +918,12 @@ def _tsdb_lookup_single_db_shard(cls, items, start_time, end_time=None, scope="D suffix = cls.column_suffix_for_value_type(value_type) col_name = f"T{index}.{safe_item_name}{suffix}" - names.append(f'"{col_name}"') + add_column(col_name) item_types[col_name] = cls.resolve_item_type(item_def, value_type) current_position += 1 if limits: - names.append(f'"T{index}.{safe_item_name}__L"') + add_column(f"T{index}.{safe_item_name}__L") # Every item in this db_shard is a placeholder so there's no table to query. # Return no results and let tsdb_lookup fill these positions with [None, None] @@ -947,7 +963,8 @@ def _tsdb_lookup_single_db_shard(cls, items, start_time, end_time=None, scope="D data.append([]) col_index = 0 row_timestamps = {} - for col_name, col_value in row.items(): + for result_name, col_value in row.items(): + col_name = aliases.get(result_name, result_name) if "__L" in col_name: if col_index > 0: data[row_index][col_index - 1] = [ diff --git a/openc3/python/test/api/test_tlm_api.py b/openc3/python/test/api/test_tlm_api.py index c3d6a464c6..fc534fad6b 100644 --- a/openc3/python/test/api/test_tlm_api.py +++ b/openc3/python/test/api/test_tlm_api.py @@ -845,6 +845,99 @@ def test_get_tlm_values_historical_passes_the_scope_to_the_tsdb_lookup(self): ) self.assertEqual(tsdb_lookup.call_args.kwargs["scope"], "OTHER") + def _latest_history_item(self, packet_name, item_name, value_type="CONVERTED"): + return TlmItem("INST", packet_name, item_name, value_type, None) + + @patch.object(TargetModel, "get_item_to_packet_map", return_value={"TEMP1": ["HEALTH_STATUS", "PARAMS"]}) + def test_get_tlm_values_historical_latest_queries_each_candidate_at_a_point_in_time(self, _): + item = self._latest_history_item + lookups = { + (item("HEALTH_STATUS", "TEMP2"),): [[[5.0, None]]], + (item("HEALTH_STATUS", "TEMP1"), item("HEALTH_STATUS", "PACKET_TIMESECONDS", "RAW")): [ + [[10.0, None], [100.0, None]] + ], + (item("PARAMS", "TEMP1"), item("PARAMS", "PACKET_TIMESECONDS", "RAW")): [[[20.0, "RED"], [200.0, None]]], + } + + def tsdb_lookup(query, start_time, scope, flatten): + self.assertFalse(flatten) + return lookups[tuple(query)] + + with patch.object(CvtModel, "tsdb_lookup", side_effect=tsdb_lookup): + vals = get_tlm_values( + ["INST__LATEST__TEMP1__CONVERTED", "INST__HEALTH_STATUS__TEMP2__CONVERTED"], + start_time="2026-09-13T00:00:00Z", + ) + self.assertEqual(vals, [[20.0, "RED"], [5.0, None]]) + + @patch.object(TargetModel, "get_item_to_packet_map", return_value={"TEMP1": ["HEALTH_STATUS", "PARAMS"]}) + def test_get_tlm_values_historical_latest_ignores_candidates_without_data(self, _): + def tsdb_lookup(query, **_kw): + return {} if query[0].packet_name == "PARAMS" else [[[10.0, None], [100.0, None]]] + + with patch.object(CvtModel, "tsdb_lookup", side_effect=tsdb_lookup): + vals = get_tlm_values(["INST__LATEST__TEMP1__CONVERTED"], start_time="2026-09-13T00:00:00Z") + self.assertEqual(vals, [[10.0, None]]) + + @patch.object(TargetModel, "get_item_to_packet_map", return_value={"TEMP1": ["HEALTH_STATUS", "PARAMS"]}) + def test_get_tlm_values_historical_latest_returns_nothing_when_nothing_is_found(self, _): + with patch.object(CvtModel, "tsdb_lookup", return_value={}): + vals = get_tlm_values( + ["INST__LATEST__TEMP1__CONVERTED", "INST__HEALTH_STATUS__TEMP2__CONVERTED"], + start_time="2026-09-13T00:00:00Z", + ) + self.assertEqual(vals, {}) + + @patch.object(TargetModel, "get_item_to_packet_map", return_value={"TEMP1": ["HEALTH_STATUS", "PARAMS"]}) + def test_get_tlm_values_historical_latest_joins_candidates_to_regular_rows_in_a_range(self, _): + item = self._latest_history_item + with patch.object(CvtModel, "tsdb_lookup") as tsdb_lookup: + tsdb_lookup.return_value = [ + [[5.0, None], [10.0, None], [300.0, None], [20.0, None], [200.0, None]], + [[6.0, None], [11.0, None], [300.0, None], [21.0, None], [400.0, None]], + [[7.0, None], [None, None], [None, None], [None, None], [None, None]], + ] + vals = get_tlm_values( + ["INST__LATEST__TEMP1__CONVERTED", "INST__HEALTH_STATUS__TEMP2__CONVERTED"], + start_time="2026-09-13T00:00:00Z", + end_time="2026-09-13T01:00:00Z", + ) + self.assertEqual( + tsdb_lookup.call_args[0][0], + [ + item("HEALTH_STATUS", "TEMP2"), + item("HEALTH_STATUS", "TEMP1"), + item("HEALTH_STATUS", "PACKET_TIMESECONDS", "RAW"), + item("PARAMS", "TEMP1"), + item("PARAMS", "PACKET_TIMESECONDS", "RAW"), + ], + ) + self.assertFalse(tsdb_lookup.call_args.kwargs["flatten"]) + self.assertEqual(vals, [[[10.0, None], [5.0, None]], [[21.0, None], [6.0, None]], [[None, None], [7.0, None]]]) + + @patch.object(TargetModel, "get_item_to_packet_map", return_value={"TEMP1": ["HEALTH_STATUS", "PARAMS"]}) + def test_get_tlm_values_historical_latest_range_of_only_latest_uses_the_current_packet(self, _): + with ( + patch.object(CvtModel, "determine_latest_packet_for_item", return_value="PARAMS"), + patch.object(CvtModel, "tsdb_lookup", return_value=[]) as tsdb_lookup, + ): + get_tlm_values( + ["INST__LATEST__TEMP1__CONVERTED"], + start_time="2026-09-13T00:00:00Z", + end_time="2026-09-13T01:00:00Z", + ) + self.assertEqual(tsdb_lookup.call_args[0][0], [self._latest_history_item("PARAMS", "TEMP1")]) + self.assertNotIn("flatten", tsdb_lookup.call_args.kwargs) + + @patch.object(TargetModel, "get_item_to_packet_map", return_value={"TEMP1": ["HEALTH_STATUS", "PARAMS"]}) + @patch("openc3.api.tlm_api.authorize") + def test_get_tlm_values_historical_latest_authorizes_every_candidate(self, mock_authorize, _): + with patch.object(CvtModel, "tsdb_lookup", return_value={}): + get_tlm_values(["INST__LATEST__TEMP1__CONVERTED"], start_time="2026-09-13T00:00:00Z") + self.assertCountEqual( + [call.kwargs["packet_name"] for call in mock_authorize.call_args_list], ["HEALTH_STATUS", "PARAMS"] + ) + def test_get_tlm_values_accepts_the_limits_suffix_from_the_cvt(self): vals = get_tlm_values(["INST__HEALTH_STATUS__TEMP1__CONVERTED__LIMITS"]) self.assertEqual(vals[0][0], (-100.0)) diff --git a/openc3/python/test/utilities/test_questdb_client.py b/openc3/python/test/utilities/test_questdb_client.py index bd8e60b046..6af6144ecd 100644 --- a/openc3/python/test/utilities/test_questdb_client.py +++ b/openc3/python/test/utilities/test_questdb_client.py @@ -34,6 +34,21 @@ def test_returns_a_row_of_nones_for_a_placeholder_without_an_end_time(self): [[None, None]], ) + def test_returns_a_list_of_rows_for_placeholders_when_not_flattening(self): + self.assertEqual( + QuestDBClient.tsdb_lookup([[None] * 5], start_time="2026-09-13T00:00:00Z", flatten=False), + [[[None, None]]], + ) + + def test_passes_flatten_to_a_single_db_shard_lookup(self): + with patch.object(QuestDBClient, "_tsdb_lookup_single_db_shard", return_value=[[[1.0, None]]]) as single: + QuestDBClient.tsdb_lookup( + [["INST", "HEALTH_STATUS", "TEMP1", "CONVERTED", None]], + start_time="2026-09-13T00:00:00Z", + flatten=False, + ) + self.assertFalse(single.call_args.kwargs["flatten"]) + class TestTsdbLookupDbShards(unittest.TestCase): ITEMS = [ @@ -78,6 +93,10 @@ def test_queries_each_db_shard_and_merges_a_single_row(self): for call in single.call_args_list: self.assertFalse(call.kwargs["flatten"]) + def test_keeps_a_single_merged_row_as_a_list_of_rows_when_not_flattening(self): + result, _ = self.lookup({0: [[[1.0, None], [3.0, None]]], 1: [[[2.0, None]]]}, flatten=False) + self.assertEqual(result, [[[1.0, None], [2.0, None], [3.0, None]]]) + def test_merges_multiple_rows_filling_missing_rows_with_nones(self): result, _ = self.lookup( {0: [[[1.0, None], [3.0, None]], [[4.0, None], [6.0, None]]], 1: [[[2.0, None]]]}, diff --git a/openc3/spec/api/tlm_api_spec.rb b/openc3/spec/api/tlm_api_spec.rb index 4a199d95f2..d2de364483 100644 --- a/openc3/spec/api/tlm_api_spec.rb +++ b/openc3/spec/api/tlm_api_spec.rb @@ -797,6 +797,78 @@ def test_tlm_unknown(method) expect(vals).to eql([[0.0, nil], [-100.0, "RED_LOW"], [nil, nil]]) end + context "with a historical LATEST item" do + before(:each) do + allow(TargetModel).to receive(:get_item_to_packet_map).and_return({ 'TEMP1' => ['HEALTH_STATUS', 'PARAMS'] }) + end + + def item(packet_name, item_name, value_type = "CONVERTED") + ["INST", packet_name, item_name, value_type, nil] + end + + let(:items) { ["INST__LATEST__TEMP1__CONVERTED", "INST__HEALTH_STATUS__TEMP2__CONVERTED"] } + + it "queries each candidate packet at a point in time and returns the newest" do + lookups = { + [item("HEALTH_STATUS", "TEMP2")] => [[[5.0, nil]]], + [item("HEALTH_STATUS", "TEMP1"), item("HEALTH_STATUS", "PACKET_TIMESECONDS", "RAW")] => [[[10.0, nil], [100.0, nil]]], + [item("PARAMS", "TEMP1"), item("PARAMS", "PACKET_TIMESECONDS", "RAW")] => [[[20.0, "RED"], [200.0, nil]]], + } + allow(CvtModel).to receive(:tsdb_lookup) do |query, start_time:, scope:, flatten:| + expect(start_time).to eql("2026-09-13T00:00:00Z") + expect(flatten).to be false + lookups.fetch(query) + end + expect(@api.get_tlm_values(items, start_time: "2026-09-13T00:00:00Z")).to eql([[20.0, "RED"], [5.0, nil]]) + end + + it "ignores candidate packets without data at a point in time" do + allow(CvtModel).to receive(:tsdb_lookup) do |query, **| + query[0][1] == "PARAMS" ? {} : [[[10.0, nil], [100.0, nil]]] + end + expect(@api.get_tlm_values(["INST__LATEST__TEMP1__CONVERTED"], start_time: "2026-09-13T00:00:00Z")).to eql([[10.0, nil]]) + end + + it "returns no results when nothing is found at a point in time" do + allow(CvtModel).to receive(:tsdb_lookup).and_return({}) + expect(@api.get_tlm_values(items, start_time: "2026-09-13T00:00:00Z")).to eql({}) + end + + it "joins the candidates to the rows of the regular items in a range" do + expect(CvtModel).to receive(:tsdb_lookup).with( + [ + item("HEALTH_STATUS", "TEMP2"), + item("HEALTH_STATUS", "TEMP1"), + item("HEALTH_STATUS", "PACKET_TIMESECONDS", "RAW"), + item("PARAMS", "TEMP1"), + item("PARAMS", "PACKET_TIMESECONDS", "RAW"), + ], + start_time: "2026-09-13T00:00:00Z", end_time: "2026-09-13T01:00:00Z", scope: "DEFAULT", flatten: false + ).and_return([ + [[5.0, nil], [10.0, nil], [300.0, nil], [20.0, nil], [200.0, nil]], + [[6.0, nil], [11.0, nil], [300.0, nil], [21.0, nil], [400.0, nil]], + [[7.0, nil], [nil, nil], [nil, nil], [nil, nil], [nil, nil]], + ]) + vals = @api.get_tlm_values(items, start_time: "2026-09-13T00:00:00Z", end_time: "2026-09-13T01:00:00Z") + expect(vals).to eql([[[10.0, nil], [5.0, nil]], [[21.0, nil], [6.0, nil]], [[nil, nil], [7.0, nil]]]) + end + + it "uses the packet received most recently now for a range of only LATEST items" do + allow(CvtModel).to receive(:determine_latest_packet_for_item).and_return("PARAMS") + expect(CvtModel).to receive(:tsdb_lookup).with( + [item("PARAMS", "TEMP1")], start_time: "2026-09-13T00:00:00Z", end_time: "2026-09-13T01:00:00Z", scope: "DEFAULT" + ).and_return([[[[20.0, nil]]], [[[21.0, nil]]]]) + @api.get_tlm_values(["INST__LATEST__TEMP1__CONVERTED"], start_time: "2026-09-13T00:00:00Z", end_time: "2026-09-13T01:00:00Z") + end + + it "authorizes every candidate packet" do + allow(CvtModel).to receive(:tsdb_lookup).and_return({}) + expect(@api).to receive(:authorize).with(hash_including(packet_name: "HEALTH_STATUS")) + expect(@api).to receive(:authorize).with(hash_including(packet_name: "PARAMS")) + @api.get_tlm_values(["INST__LATEST__TEMP1__CONVERTED"], start_time: "2026-09-13T00:00:00Z") + end + end + it "accepts the limits suffix from get_tlm_available" do vals = @api.get_tlm_values(["INST__HEALTH_STATUS__TEMP1__CONVERTED__LIMITS"]) expect(vals[0][0]).to eql(-100.0) diff --git a/openc3/spec/utilities/questdb_client_spec.rb b/openc3/spec/utilities/questdb_client_spec.rb index 1f01e1d4b7..888d356b8b 100644 --- a/openc3/spec/utilities/questdb_client_spec.rb +++ b/openc3/spec/utilities/questdb_client_spec.rb @@ -29,6 +29,16 @@ module OpenC3 expect(QuestDBClient.tsdb_lookup([Array.new(5)], start_time: "2026-09-13T00:00:00Z")).to eq([[nil, nil]]) end + it "returns a list of rows for placeholders when not flattening" do + expect(QuestDBClient.tsdb_lookup([Array.new(5)], start_time: "2026-09-13T00:00:00Z", flatten: false)).to eq([[[nil, nil]]]) + end + + it "passes flatten to a single db_shard lookup" do + allow(QuestDBClient).to receive(:tsdb_lookup_single_db_shard).and_return([[[1.0, nil]]]) + QuestDBClient.tsdb_lookup([["INST", "HEALTH_STATUS", "TEMP1", "CONVERTED", nil]], start_time: "2026-09-13T00:00:00Z", flatten: false) + expect(QuestDBClient).to have_received(:tsdb_lookup_single_db_shard).with(anything, hash_including(flatten: false)) + end + context "with items across db_shards" do let(:items) do [ @@ -38,14 +48,14 @@ module OpenC3 ] end - def lookup(results, end_time: nil) + def lookup(results, end_time: nil, flatten: true) # INST is on db_shard 0 and INST2 is on db_shard 1 allow(QuestDBClient).to receive(:db_shard_for_target) { |target_name, **| target_name == "INST2" ? 1 : 0 } allow(QuestDBClient).to receive(:tsdb_lookup_single_db_shard) do |_items, db_shard:, flatten:, **| expect(flatten).to be false results[db_shard] end - QuestDBClient.tsdb_lookup(items, start_time: "2026-09-13T00:00:00Z", end_time: end_time) + QuestDBClient.tsdb_lookup(items, start_time: "2026-09-13T00:00:00Z", end_time: end_time, flatten: flatten) end it "queries each db_shard and merges a single row" do @@ -55,6 +65,11 @@ def lookup(results, end_time: nil) expect(QuestDBClient).to have_received(:tsdb_lookup_single_db_shard).with([items[1]], hash_including(db_shard: 1)) end + it "keeps a single merged row as a list of rows when not flattening" do + result = lookup({ 0 => [[[1.0, nil], [3.0, nil]]], 1 => [[[2.0, nil]]] }, flatten: false) + expect(result).to eq([[[1.0, nil], [2.0, nil], [3.0, nil]]]) + end + it "merges a single row of a multi-row query" do # A db_shard with a single row must not be mistaken for multiple rows result = lookup({ 0 => [[[1.0, nil], [3.0, nil]]], 1 => [[[2.0, nil]]] }, end_time: "2026-09-13T01:00:00Z") diff --git a/openc3/test/integration/tsdb/python/test_tlm_api_latest_tsdb.py b/openc3/test/integration/tsdb/python/test_tlm_api_latest_tsdb.py new file mode 100644 index 0000000000..859b4aeacc --- /dev/null +++ b/openc3/test/integration/tsdb/python/test_tlm_api_latest_tsdb.py @@ -0,0 +1,115 @@ +# Copyright 2026 OpenC3, Inc. +# All Rights Reserved. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. +# See LICENSE.md for more details. +# +# This file may also be used under the terms of a commercial license +# if purchased from OpenC3, Inc. + +""" +End-to-end tests for historical get_tlm_values with LATEST items. + +LATEST must return the value from whichever packet containing the item was +received most recently at the requested time, not the packet received most +recently now, and not just the first packet in the QuestDB ASOF JOIN. + +Run with: + 1. Start QuestDB: docker compose -f docker-compose.test.yml up -d + 2. Run tests: uv run pytest ../test/integration/tsdb/python/test_tlm_api_latest_tsdb.py (from openc3/python) + 3. Stop QuestDB: docker compose -f docker-compose.test.yml down +""" + +import os +import sys +import time +from datetime import datetime, timezone +from unittest.mock import patch + +import pytest + +# Add openc3 python path for imports +# Path: openc3/test/integration/tsdb/python -> openc3/python +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "..", "..", "..", "python")) + +os.environ.setdefault("OPENC3_TSDB_HOSTNAME", "127.0.0.1") +os.environ.setdefault("OPENC3_TSDB_INGEST_PORT", "9000") +os.environ.setdefault("OPENC3_TSDB_QUERY_PORT", "8812") +os.environ.setdefault("OPENC3_TSDB_USERNAME", "admin") +os.environ.setdefault("OPENC3_TSDB_PASSWORD", "admin") +os.environ.setdefault("OPENC3_SCOPE", "DEFAULT") + +from conftest import requires_questdb +from openc3.api.tlm_api import get_tlm_values +from openc3.models.target_model import TargetModel +from openc3.utilities.questdb_client import QuestDBClient + +TARGET = "PY_LT" +PACKET_DEF = {"items": [{"name": "VALUE", "data_type": "INT", "bit_size": 32}]} +# Timeline in ms from the base time (VALUE written to each packet) +ROWS = { + "PKT_A": [(0, 1), (2000, 3)], + "PKT_B": [(1000, 2), (3000, 4)], + "REGULAR": [(500, 10), (1500, 20), (2500, 30), (3500, 40)], +} + + +def ts_to_iso(ts_ns): + return datetime.fromtimestamp(ts_ns / 1e9, tz=timezone.utc).strftime("%Y-%m-%dT%H:%M:%S.%fZ") + + +@requires_questdb +class TestTlmApiLatestTsdb: + @pytest.fixture + def base_ns(self, questdb_client, clean_table, wait_for_data): + base_ns = int(time.time() * 1e9) + for packet_name, rows in ROWS.items(): + table_name, _ = QuestDBClient.sanitize_table_name(TARGET, packet_name) + clean_table(table_name) + questdb_client.create_table(TARGET, packet_name, PACKET_DEF) + for i, (offset_ms, value) in enumerate(rows): + columns = questdb_client.process_json_data({"VALUE": value}, table_name) + columns["RECEIVED_COUNT"] = i + questdb_client.write_row(table_name, columns, base_ns + offset_ms * 1_000_000) + questdb_client.flush() + for packet_name, rows in ROWS.items(): + table_name, _ = QuestDBClient.sanitize_table_name(TARGET, packet_name) + assert wait_for_data(questdb_client, table_name, len(rows)) >= len(rows) + with ( + patch.object(TargetModel, "packet", return_value=PACKET_DEF), + patch.object(TargetModel, "get_item_to_packet_map", return_value={"VALUE": ["PKT_A", "PKT_B"]}), + patch("openc3.api.tlm_api.authorize"), + ): + yield base_ns + QuestDBClient.disconnect() + + def at(self, base_ns, offset_ms): + return ts_to_iso(base_ns + offset_ms * 1_000_000) + + def test_returns_the_packet_received_most_recently_at_a_point_in_time(self, base_ns): + assert get_tlm_values([f"{TARGET}__LATEST__VALUE__RAW"], start_time=self.at(base_ns, 2500)) == [[3, None]] + + def test_returns_a_packet_received_after_the_last_row_of_the_first_candidate(self, base_ns): + # PKT_A was last received at 2000 but PKT_B at 3000 so PKT_B is the latest at 3500 + assert get_tlm_values([f"{TARGET}__LATEST__VALUE__RAW"], start_time=self.at(base_ns, 3500)) == [[4, None]] + + def test_returns_latest_alongside_a_regular_item_at_a_point_in_time(self, base_ns): + vals = get_tlm_values( + [f"{TARGET}__LATEST__VALUE__RAW", f"{TARGET}__REGULAR__VALUE__RAW"], start_time=self.at(base_ns, 3600) + ) + assert vals == [[4, None], [40, None]] + + def test_returns_the_latest_packet_at_each_row_of_a_range_driven_by_a_regular_item(self, base_ns): + vals = get_tlm_values( + [f"{TARGET}__LATEST__VALUE__RAW", f"{TARGET}__REGULAR__VALUE__RAW"], + start_time=self.at(base_ns, 0), + end_time=self.at(base_ns, 4000), + ) + assert vals == [ + [[1, None], [10, None]], + [[2, None], [20, None]], + [[3, None], [30, None]], + [[4, None], [40, None]], + ] diff --git a/openc3/test/integration/tsdb/ruby/tlm_api_latest_tsdb_spec.rb b/openc3/test/integration/tsdb/ruby/tlm_api_latest_tsdb_spec.rb new file mode 100644 index 0000000000..a37fb9bd5d --- /dev/null +++ b/openc3/test/integration/tsdb/ruby/tlm_api_latest_tsdb_spec.rb @@ -0,0 +1,93 @@ +# Copyright 2026 OpenC3, Inc. +# All Rights Reserved. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. +# See LICENSE.md for more details. + +# This file may also be used under the terms of a commercial license +# if purchased from OpenC3, Inc. + +# End-to-end tests for historical get_tlm_values with LATEST items. +# +# LATEST must return the value from whichever packet containing the item was +# received most recently at the requested time, not the packet received most +# recently now, and not just the first packet in the QuestDB ASOF JOIN. +# +# Run with: +# 1. Start QuestDB: docker compose -f docker-compose.test.yml up -d +# 2. Run tests: bundle exec rspec ruby/tlm_api_latest_tsdb_spec.rb +# 3. Stop QuestDB: docker compose -f docker-compose.test.yml down + +require_relative 'spec_helper' +require 'openc3/api/tlm_api' +require 'openc3/utilities/authorization' + +RSpec.describe 'get_tlm_values with historical LATEST', :questdb do + class LatestApi + include OpenC3::Api + include OpenC3::Authorization + end + + # Timeline in ms from the base time (VALUE written to each packet): + # PKT_A: 0 => 1, 2000 => 3 + # PKT_B: 1000 => 2, 3000 => 4 + # REGULAR: 500 => 10, 1500 => 20, 2500 => 30, 3500 => 40 + before(:all) do + @test_params = write_multi_table_data({ + 'tables' => [ + { 'target' => 'RUBY_LT', 'packet' => 'PKT_A', 'data_type' => 'INT', 'bit_size' => 32, + 'rows' => [{ 'offset_ms' => 0, 'values' => { 'VALUE' => 1 } }, { 'offset_ms' => 2000, 'values' => { 'VALUE' => 3 } }] }, + { 'target' => 'RUBY_LT', 'packet' => 'PKT_B', 'data_type' => 'INT', 'bit_size' => 32, + 'rows' => [{ 'offset_ms' => 1000, 'values' => { 'VALUE' => 2 } }, { 'offset_ms' => 3000, 'values' => { 'VALUE' => 4 } }] }, + { 'target' => 'RUBY_LT', 'packet' => 'REGULAR', 'data_type' => 'INT', 'bit_size' => 32, + 'rows' => [500, 1500, 2500, 3500].each_with_index.map { |offset, i| { 'offset_ms' => offset, 'values' => { 'VALUE' => (i + 1) * 10 } } } }, + ] + }) + end + + before(:each) do + OpenC3::QuestDBClient.disconnect + expect(@test_params['success']).to be true + packet_defs = @test_params['tables'].to_h { |table| [table['packet_name'], table['packet_def']] } + allow(OpenC3::TargetModel).to receive(:packet) { |_target_name, packet_name, **| packet_defs.fetch(packet_name) } + allow(OpenC3::TargetModel).to receive(:get_item_to_packet_map).and_return({ 'VALUE' => ['PKT_A', 'PKT_B'] }) + @api = LatestApi.new + allow(@api).to receive(:authorize) + end + + after(:each) do + OpenC3::QuestDBClient.disconnect + end + + def time_at(offset_ms) + ns = @test_params['base_time_ns'] + (offset_ms * 1_000_000) + Time.at(ns / 1_000_000_000, ns % 1_000_000_000, :nsec).utc.strftime('%Y-%m-%dT%H:%M:%S.%6NZ') + end + + it 'returns the packet received most recently at a point in time' do + expect(@api.get_tlm_values(['RUBY_LT__LATEST__VALUE__RAW'], start_time: time_at(2500))).to eq([[3, nil]]) + end + + it 'returns a packet received after the last row of the first candidate' do + # PKT_A was last received at 2000 but PKT_B at 3000 so PKT_B is the latest at 3500 + expect(@api.get_tlm_values(['RUBY_LT__LATEST__VALUE__RAW'], start_time: time_at(3500))).to eq([[4, nil]]) + end + + it 'returns LATEST alongside a regular item at a point in time' do + vals = @api.get_tlm_values(['RUBY_LT__LATEST__VALUE__RAW', 'RUBY_LT__REGULAR__VALUE__RAW'], start_time: time_at(3600)) + expect(vals).to eq([[4, nil], [40, nil]]) + end + + it 'returns the latest packet at each row of a range driven by a regular item' do + vals = @api.get_tlm_values(['RUBY_LT__LATEST__VALUE__RAW', 'RUBY_LT__REGULAR__VALUE__RAW'], + start_time: time_at(0), end_time: time_at(4000)) + expect(vals).to eq([ + [[1, nil], [10, nil]], + [[2, nil], [20, nil]], + [[3, nil], [30, nil]], + [[4, nil], [40, nil]], + ]) + end +end From aced396cfa5a48409a571caaff25a02a7e180ab9 Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Mon, 12 Oct 2026 17:19:48 -0600 Subject: [PATCH 05/14] fix(telemetry): check each LATEST packet for available value types - Historical LATEST queries each candidate packet with the value type and limits that packet has; asking for a missing column (VALUE__F) failed the whole QuestDB query - get_tlm_available reports the best value type and limits across all LATEST packets instead of only the one received most recently - Factor _tlm_available_value_type out of get_tlm_available Co-Authored-By: Claude Opus 5.5 --- openc3/lib/openc3/api/tlm_api.rb | 89 ++++++++++--------- openc3/python/openc3/api/tlm_api.py | 80 ++++++++++------- openc3/python/test/api/test_tlm_api.py | 75 +++++++++++++--- openc3/spec/api/tlm_api_spec.rb | 25 ++++++ .../tsdb/python/test_tlm_api_latest_tsdb.py | 26 +++++- .../tsdb/ruby/tlm_api_latest_tsdb_spec.rb | 22 ++++- 6 files changed, 222 insertions(+), 95 deletions(-) diff --git a/openc3/lib/openc3/api/tlm_api.rb b/openc3/lib/openc3/api/tlm_api.rb index 415239b4de..71fe6e0c33 100644 --- a/openc3/lib/openc3/api/tlm_api.rb +++ b/openc3/lib/openc3/api/tlm_api.rb @@ -275,45 +275,22 @@ def get_tlm_available(items, manual: false, scope: $openc3_scope, token: $openc3 packet_name = orig_packet_name raise ArgumentError, "items must be formatted as TGT__PKT__ITEM__TYPE" if target_name.nil? || packet_name.nil? || item_name.nil? || value_type.nil? if orig_packet_name == 'LATEST' - # TODO: Do we need to lookup ALL the possible packets for this item? + # A LATEST item can come from any packet containing it. Report the best value + # type and limits of any of them since the CVT falls back to the available + # type and historical lookups check the type available in each packet. # We can have a large cache_timeout of 1 because all we're trying to do is lookup a packet packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout: 1, scope: scope) + packet_names = _tlm_packet_names(target_name, orig_packet_name, item_name, scope: scope) + else + packet_names = [packet_name] end authorize(permission: 'tlm', target_name: target_name, packet_name: packet_name, scope: scope, token: token) begin - item = TargetModel.packet_item(target_name, packet_name, item_name, scope: scope) - if Packet::RESERVED_ITEM_NAMES.include?(item_name) - value_type = 'RAW' # Must request the raw value when dealing with the reserved items - end - - # Arrays must be accessed as RAW since there's no conversion - if item['array_size'] - value_type = 'RAW' - end - - case value_type - when 'FORMATTED', 'WITH_UNITS' - if item['format_string'] or item['units'] - results << [target_name, orig_packet_name, item_name, 'FORMATTED'].join('__') - # This logic must match the logic in Packet#decom - elsif item['states'] or (item['read_conversion'] and item['data_type'] != 'DERIVED') - results << [target_name, orig_packet_name, item_name, 'CONVERTED'].join('__') - else - results << [target_name, orig_packet_name, item_name, 'RAW'].join('__') - end - when 'CONVERTED' - # This logic must match the logic in Packet#decom - if item['states'] or (item['read_conversion'] and item['data_type'] != 'DERIVED') - results << [target_name, orig_packet_name, item_name, 'CONVERTED'].join('__') - else - results << [target_name, orig_packet_name, item_name, 'RAW'].join('__') - end - else # RAW or unknown - results << [target_name, orig_packet_name, item_name, 'RAW'].join('__') - end - + packet_items = packet_names.map { |name| TargetModel.packet_item(target_name, name, item_name, scope: scope) } + available_type = packet_items.map { |packet_item| _tlm_available_value_type(packet_item, item_name, value_type) }.max_by { |type| VALUE_TYPE_ORDER.index(type) } + results << [target_name, orig_packet_name, item_name, available_type].join('__') # Tack on __LIMITS to notify that we have an available limits value - if item['limits']['DEFAULT'] + if packet_items.any? { |packet_item| packet_item['limits']['DEFAULT'] } results[-1] += '__LIMITS' end rescue RuntimeError @@ -355,8 +332,13 @@ def get_tlm_values(items, stale_time: 30, cache_timeout: nil, manual: false, sta if start_time # The packet received most recently now isn't necessarily the one received most # recently at start_time so keep every candidate packet for _tlm_latest_history - candidates = _tlm_packet_names(target_name, packet_name, item_name, scope: scope) - candidates.each { |candidate| packets << [target_name, candidate] } + # Each packet must be queried for the value type and limits it has or the query fails + candidates = _tlm_packet_names(target_name, packet_name, item_name, scope: scope).map do |candidate| + packets << [target_name, candidate] + candidate_item = TargetModel.packet_item(target_name, candidate, item_name, scope: scope) + candidate_limits = (limits and candidate_item['limits']['DEFAULT']) ? limits : nil + [target_name, candidate, item_name, _tlm_available_value_type(candidate_item, item_name, value_type), candidate_limits] + end cvt_items << [target_name, packet_name, item_name, value_type, limits, candidates] next end @@ -680,8 +662,30 @@ def _tlm_resolve_packet_name(target_name, packet_name, item_name, cache_timeout: return packet_name end + VALUE_TYPE_ORDER = %w(RAW CONVERTED FORMATTED).freeze + + # Returns the value type available for an item given the requested value type + def _tlm_available_value_type(item, item_name, value_type) + # Must request the raw value when dealing with the reserved items + return 'RAW' if Packet::RESERVED_ITEM_NAMES.include?(item_name) + # Arrays must be accessed as RAW since there's no conversion + return 'RAW' if item['array_size'] + + # This logic must match the logic in Packet#decom + converted = (item['states'] or (item['read_conversion'] and item['data_type'] != 'DERIVED')) + case value_type + when 'FORMATTED', 'WITH_UNITS' + return 'FORMATTED' if item['format_string'] or item['units'] + return converted ? 'CONVERTED' : 'RAW' + when 'CONVERTED' + return converted ? 'CONVERTED' : 'RAW' + else # RAW or unknown + return 'RAW' + end + end + # Historical lookup where some items are LATEST. Those items have a 6th element - # holding every packet which contains the item. + # holding the item to look up in every packet which contains the item. def _tlm_latest_history(cvt_items, start_time:, end_time:, cache_timeout:, scope:) latest = cvt_items.each_index.select { |index| cvt_items[index].length == 6 } regular = cvt_items.each_index.reject { |index| cvt_items[index].length == 6 } @@ -704,10 +708,9 @@ def _tlm_latest_history(cvt_items, start_time:, end_time:, cache_timeout:, scope positions = Array.new(cvt_items.length) regular.each { |index| positions[index] = add.call(cvt_items[index]) } latest.each do |index| - target_name, _, item_name, value_type, limits, candidates = cvt_items[index] - positions[index] = candidates.map do |candidate| - [add.call([target_name, candidate, item_name, value_type, limits]), - add.call([target_name, candidate, 'PACKET_TIMESECONDS', 'RAW', nil])] + positions[index] = cvt_items[index][5].map do |candidate| + target_name, packet_name = candidate + [add.call(candidate), add.call([target_name, packet_name, 'PACKET_TIMESECONDS', 'RAW', nil])] end end rows = CvtModel.tsdb_lookup(query, start_time: start_time, end_time: end_time, scope: scope, flatten: false) @@ -738,9 +741,9 @@ def _tlm_latest_history(cvt_items, start_time:, end_time:, cache_timeout:, scope # Group the LATEST items by candidate packet so each packet is queried once by_packet = {} latest.each do |index| - target_name, _, item_name, value_type, limits, candidates = cvt_items[index] - candidates.each do |candidate| - (by_packet[[target_name, candidate]] ||= {})[index] = [target_name, candidate, item_name, value_type, limits] + cvt_items[index][5].each do |candidate| + target_name, packet_name = candidate + (by_packet[[target_name, packet_name]] ||= {})[index] = candidate end end found_values = Hash.new { |hash, key| hash[key] = [] } diff --git a/openc3/python/openc3/api/tlm_api.py b/openc3/python/openc3/api/tlm_api.py index 94c8d585ad..f09eff1c05 100644 --- a/openc3/python/openc3/api/tlm_api.py +++ b/openc3/python/openc3/api/tlm_api.py @@ -325,10 +325,16 @@ def get_tlm_available(items, manual=False, scope=OPENC3_SCOPE): packet_name = orig_packet_name if orig_packet_name == "LATEST": + # A LATEST item can come from any packet containing it. Report the best value + # type and limits of any of them since the CVT falls back to the available + # type and historical lookups check the type available in each packet. # We can have a large cache_timeout of 1 because all we're trying to do is lookup a packet packet_name = CvtModel.determine_latest_packet_for_item( target_name, item_name, cache_timeout=1, scope=scope ) + packet_names = _tlm_packet_names(target_name, orig_packet_name, item_name, scope) + else: + packet_names = [packet_name] authorize( permission="tlm", @@ -338,32 +344,15 @@ def get_tlm_available(items, manual=False, scope=OPENC3_SCOPE): ) try: - item_config = TargetModel.packet_item(target_name, packet_name, item_name, scope=scope) - if item_name in Packet.RESERVED_ITEM_NAMES: - value_type = "RAW" # Must request the raw value when dealing with the reserved items - - # Arrays must be accessed as RAW since there's no conversion - if item_config.get("array_size") is not None: - value_type = "RAW" - - # Determine the best available value type based on item configuration - if value_type == "FORMATTED" or value_type == "WITH_UNITS": - if item_config.get("format_string") or item_config.get("units"): - result_item = "__".join([target_name, orig_packet_name, item_name, "FORMATTED"]) - elif item_config.get("read_conversion") or item_config.get("states"): - result_item = "__".join([target_name, orig_packet_name, item_name, "CONVERTED"]) - else: - result_item = "__".join([target_name, orig_packet_name, item_name, "RAW"]) - elif value_type == "CONVERTED": - if item_config.get("read_conversion") or item_config.get("states"): - result_item = "__".join([target_name, orig_packet_name, item_name, "CONVERTED"]) - else: - result_item = "__".join([target_name, orig_packet_name, item_name, "RAW"]) - else: # RAW or unknown - result_item = "__".join([target_name, orig_packet_name, item_name, "RAW"]) + item_configs = [TargetModel.packet_item(target_name, name, item_name, scope=scope) for name in packet_names] + available_type = max( + (_tlm_available_value_type(item_config, item_name, value_type) for item_config in item_configs), + key=VALUE_TYPE_ORDER.index, + ) + result_item = "__".join([target_name, orig_packet_name, item_name, available_type]) # Tack on __LIMITS to notify that we have an available limits value - if item_config.get("limits", {}).get("DEFAULT"): + if any(item_config.get("limits", {}).get("DEFAULT") for item_config in item_configs): result_item += "__LIMITS" results.append(result_item) @@ -415,8 +404,14 @@ def get_tlm_values( if start_time is not None: # The packet received most recently now isn't necessarily the one received most # recently at start_time so keep every candidate packet for _tlm_latest_history - candidates = _tlm_packet_names(target_name, packet_name, item_name, scope) - packets.extend([target_name, candidate] for candidate in candidates) + # Each packet must be queried for the value type and limits it has or the query fails + candidates = [] + for candidate in _tlm_packet_names(target_name, packet_name, item_name, scope): + packets.append([target_name, candidate]) + candidate_item = TargetModel.packet_item(target_name, candidate, item_name, scope=scope) + candidate_limits = limits if limits and candidate_item.get("limits", {}).get("DEFAULT") else None + candidate_type = _tlm_available_value_type(candidate_item, item_name, value_type) + candidates.append(TlmItem(target_name, candidate, item_name, candidate_type, candidate_limits)) latest_candidates[len(cvt_items)] = candidates cvt_items.append(TlmItem(target_name, packet_name, item_name, value_type, limits)) continue @@ -791,8 +786,29 @@ def _tlm_resolve_packet_name(target_name, packet_name, item_name, cache_timeout= return packet_name +VALUE_TYPE_ORDER = ["RAW", "CONVERTED", "FORMATTED"] + + +# Returns the value type available for an item given the requested value type +def _tlm_available_value_type(item_config, item_name, value_type): + # Must request the raw value when dealing with the reserved items + if item_name in Packet.RESERVED_ITEM_NAMES: + return "RAW" + # Arrays must be accessed as RAW since there's no conversion + if item_config.get("array_size") is not None: + return "RAW" + converted = item_config.get("read_conversion") or item_config.get("states") + if value_type == "FORMATTED" or value_type == "WITH_UNITS": + if item_config.get("format_string") or item_config.get("units"): + return "FORMATTED" + return "CONVERTED" if converted else "RAW" + if value_type == "CONVERTED": + return "CONVERTED" if converted else "RAW" + return "RAW" # RAW or unknown + + # Historical lookup where some items are LATEST. latest_candidates maps the index -# of each LATEST item in cvt_items to every packet which contains the item. +# of each LATEST item in cvt_items to the item to look up in every packet which contains it. def _tlm_latest_history(cvt_items, latest_candidates, start_time, end_time, cache_timeout, scope): regular = [index for index in range(len(cvt_items)) if index not in latest_candidates] @@ -824,11 +840,10 @@ def add(query_item): for index in regular: positions[index] = add(cvt_items[index]) for index, candidates in latest_candidates.items(): - cvt_item = cvt_items[index] positions[index] = [ ( - add(cvt_item._replace(packet_name=candidate)), - add(TlmItem(cvt_item.target_name, candidate, "PACKET_TIMESECONDS", "RAW", None)), + add(candidate), + add(TlmItem(candidate.target_name, candidate.packet_name, "PACKET_TIMESECONDS", "RAW", None)), ) for candidate in candidates ] @@ -867,11 +882,8 @@ def add(query_item): # Group the LATEST items by candidate packet so each packet is queried once by_packet = {} for index, candidates in latest_candidates.items(): - cvt_item = cvt_items[index] for candidate in candidates: - by_packet.setdefault((cvt_item.target_name, candidate), {})[index] = cvt_item._replace( - packet_name=candidate - ) + by_packet.setdefault((candidate.target_name, candidate.packet_name), {})[index] = candidate found_values = {} for (target_name, packet_name), packet_items in by_packet.items(): query = list(dict.fromkeys(packet_items.values())) diff --git a/openc3/python/test/api/test_tlm_api.py b/openc3/python/test/api/test_tlm_api.py index fc534fad6b..4ebc2c025a 100644 --- a/openc3/python/test/api/test_tlm_api.py +++ b/openc3/python/test/api/test_tlm_api.py @@ -31,6 +31,28 @@ from test.test_helper import System, mock_redis, setup_system +ORIGINAL_PACKET_ITEM = TargetModel.packet_item +PLAIN_TEMP1 = {"name": "TEMP1", "data_type": "UINT", "limits": {}} + + +# Make TEMP1 a LATEST item in HEALTH_STATUS and PARAMS. PARAMS TEMP1 is the +# same as HEALTH_STATUS TEMP1 unless params_temp1 is given. +def latest_candidates(params_temp1=None): + def packet_item(target_name, packet_name, item_name, type="TLM", scope="DEFAULT"): + if packet_name == "PARAMS" and item_name == "TEMP1": + return params_temp1 or ORIGINAL_PACKET_ITEM("INST", "HEALTH_STATUS", "TEMP1", scope=scope) + return ORIGINAL_PACKET_ITEM(target_name, packet_name, item_name, type=type, scope=scope) + + def item_map(target_name, scope="DEFAULT"): + return {"TEMP1": ["HEALTH_STATUS", "PARAMS"]} + + def decorator(func): + func = patch.object(TargetModel, "packet_item", new=packet_item)(func) + return patch.object(TargetModel, "get_item_to_packet_map", new=item_map)(func) + + return decorator + + class TestTlmApi(unittest.TestCase): def setUp(self): redis = mock_redis(self) @@ -848,8 +870,8 @@ def test_get_tlm_values_historical_passes_the_scope_to_the_tsdb_lookup(self): def _latest_history_item(self, packet_name, item_name, value_type="CONVERTED"): return TlmItem("INST", packet_name, item_name, value_type, None) - @patch.object(TargetModel, "get_item_to_packet_map", return_value={"TEMP1": ["HEALTH_STATUS", "PARAMS"]}) - def test_get_tlm_values_historical_latest_queries_each_candidate_at_a_point_in_time(self, _): + @latest_candidates() + def test_get_tlm_values_historical_latest_queries_each_candidate_at_a_point_in_time(self): item = self._latest_history_item lookups = { (item("HEALTH_STATUS", "TEMP2"),): [[[5.0, None]]], @@ -870,8 +892,8 @@ def tsdb_lookup(query, start_time, scope, flatten): ) self.assertEqual(vals, [[20.0, "RED"], [5.0, None]]) - @patch.object(TargetModel, "get_item_to_packet_map", return_value={"TEMP1": ["HEALTH_STATUS", "PARAMS"]}) - def test_get_tlm_values_historical_latest_ignores_candidates_without_data(self, _): + @latest_candidates() + def test_get_tlm_values_historical_latest_ignores_candidates_without_data(self): def tsdb_lookup(query, **_kw): return {} if query[0].packet_name == "PARAMS" else [[[10.0, None], [100.0, None]]] @@ -879,8 +901,8 @@ def tsdb_lookup(query, **_kw): vals = get_tlm_values(["INST__LATEST__TEMP1__CONVERTED"], start_time="2026-09-13T00:00:00Z") self.assertEqual(vals, [[10.0, None]]) - @patch.object(TargetModel, "get_item_to_packet_map", return_value={"TEMP1": ["HEALTH_STATUS", "PARAMS"]}) - def test_get_tlm_values_historical_latest_returns_nothing_when_nothing_is_found(self, _): + @latest_candidates() + def test_get_tlm_values_historical_latest_returns_nothing_when_nothing_is_found(self): with patch.object(CvtModel, "tsdb_lookup", return_value={}): vals = get_tlm_values( ["INST__LATEST__TEMP1__CONVERTED", "INST__HEALTH_STATUS__TEMP2__CONVERTED"], @@ -888,8 +910,8 @@ def test_get_tlm_values_historical_latest_returns_nothing_when_nothing_is_found( ) self.assertEqual(vals, {}) - @patch.object(TargetModel, "get_item_to_packet_map", return_value={"TEMP1": ["HEALTH_STATUS", "PARAMS"]}) - def test_get_tlm_values_historical_latest_joins_candidates_to_regular_rows_in_a_range(self, _): + @latest_candidates() + def test_get_tlm_values_historical_latest_joins_candidates_to_regular_rows_in_a_range(self): item = self._latest_history_item with patch.object(CvtModel, "tsdb_lookup") as tsdb_lookup: tsdb_lookup.return_value = [ @@ -915,8 +937,8 @@ def test_get_tlm_values_historical_latest_joins_candidates_to_regular_rows_in_a_ self.assertFalse(tsdb_lookup.call_args.kwargs["flatten"]) self.assertEqual(vals, [[[10.0, None], [5.0, None]], [[21.0, None], [6.0, None]], [[None, None], [7.0, None]]]) - @patch.object(TargetModel, "get_item_to_packet_map", return_value={"TEMP1": ["HEALTH_STATUS", "PARAMS"]}) - def test_get_tlm_values_historical_latest_range_of_only_latest_uses_the_current_packet(self, _): + @latest_candidates() + def test_get_tlm_values_historical_latest_range_of_only_latest_uses_the_current_packet(self): with ( patch.object(CvtModel, "determine_latest_packet_for_item", return_value="PARAMS"), patch.object(CvtModel, "tsdb_lookup", return_value=[]) as tsdb_lookup, @@ -929,9 +951,38 @@ def test_get_tlm_values_historical_latest_range_of_only_latest_uses_the_current_ self.assertEqual(tsdb_lookup.call_args[0][0], [self._latest_history_item("PARAMS", "TEMP1")]) self.assertNotIn("flatten", tsdb_lookup.call_args.kwargs) - @patch.object(TargetModel, "get_item_to_packet_map", return_value={"TEMP1": ["HEALTH_STATUS", "PARAMS"]}) + @latest_candidates(params_temp1=PLAIN_TEMP1) + def test_get_tlm_values_historical_latest_queries_each_candidate_for_its_type_and_limits(self): + item = self._latest_history_item + queries = [] + + def tsdb_lookup(query, **_kw): + queries.append(query) + return {} + + with patch.object(CvtModel, "tsdb_lookup", side_effect=tsdb_lookup): + get_tlm_values(["INST__LATEST__TEMP1__FORMATTED__LIMITS"], start_time="2026-09-13T00:00:00Z") + self.assertCountEqual( + queries, + [ + [ + TlmItem("INST", "HEALTH_STATUS", "TEMP1", "FORMATTED", "LIMITS"), + item("HEALTH_STATUS", "PACKET_TIMESECONDS", "RAW"), + ], + [TlmItem("INST", "PARAMS", "TEMP1", "RAW", None), item("PARAMS", "PACKET_TIMESECONDS", "RAW")], + ], + ) + + @latest_candidates(params_temp1=PLAIN_TEMP1) + def test_get_tlm_available_returns_the_best_type_and_limits_of_any_latest_packet(self): + with patch.object(CvtModel, "determine_latest_packet_for_item", return_value="PARAMS"): + self.assertEqual( + get_tlm_available(["INST__LATEST__TEMP1__FORMATTED"]), ["INST__LATEST__TEMP1__FORMATTED__LIMITS"] + ) + + @latest_candidates() @patch("openc3.api.tlm_api.authorize") - def test_get_tlm_values_historical_latest_authorizes_every_candidate(self, mock_authorize, _): + def test_get_tlm_values_historical_latest_authorizes_every_candidate(self, mock_authorize): with patch.object(CvtModel, "tsdb_lookup", return_value={}): get_tlm_values(["INST__LATEST__TEMP1__CONVERTED"], start_time="2026-09-13T00:00:00Z") self.assertCountEqual( diff --git a/openc3/spec/api/tlm_api_spec.rb b/openc3/spec/api/tlm_api_spec.rb index d2de364483..c7a28b29c1 100644 --- a/openc3/spec/api/tlm_api_spec.rb +++ b/openc3/spec/api/tlm_api_spec.rb @@ -699,6 +699,15 @@ def test_tlm_unknown(method) end describe "get_tlm_available" do + it "returns the best value type and limits of any LATEST packet" do + allow(TargetModel).to receive(:get_item_to_packet_map).and_return({ 'TEMP1' => ['PARAMS', 'HEALTH_STATUS'] }) + allow(CvtModel).to receive(:determine_latest_packet_for_item).and_return('PARAMS') + allow(TargetModel).to receive(:packet_item).and_call_original + plain = { 'name' => 'TEMP1', 'data_type' => 'UINT', 'limits' => {} } + allow(TargetModel).to receive(:packet_item).with('INST', 'PARAMS', 'TEMP1', scope: 'DEFAULT').and_return(plain) + expect(@api.get_tlm_available(['INST__LATEST__TEMP1__FORMATTED'])).to eql(['INST__LATEST__TEMP1__FORMATTED__LIMITS']) + end + it "returns a valid list of items based on inputs" do items = [] # Ask for WITH_UNITS for an item which has various formats @@ -800,6 +809,10 @@ def test_tlm_unknown(method) context "with a historical LATEST item" do before(:each) do allow(TargetModel).to receive(:get_item_to_packet_map).and_return({ 'TEMP1' => ['HEALTH_STATUS', 'PARAMS'] }) + # Pretend PARAMS has the same TEMP1 as HEALTH_STATUS + temp1 = TargetModel.packet_item('INST', 'HEALTH_STATUS', 'TEMP1', scope: 'DEFAULT') + allow(TargetModel).to receive(:packet_item).and_call_original + allow(TargetModel).to receive(:packet_item).with('INST', 'PARAMS', 'TEMP1', scope: 'DEFAULT').and_return(temp1) end def item(packet_name, item_name, value_type = "CONVERTED") @@ -861,6 +874,18 @@ def item(packet_name, item_name, value_type = "CONVERTED") @api.get_tlm_values(["INST__LATEST__TEMP1__CONVERTED"], start_time: "2026-09-13T00:00:00Z", end_time: "2026-09-13T01:00:00Z") end + it "queries each candidate for the value type and limits it has" do + plain = { 'name' => 'TEMP1', 'data_type' => 'UINT', 'limits' => {} } + allow(TargetModel).to receive(:packet_item).with('INST', 'PARAMS', 'TEMP1', scope: 'DEFAULT').and_return(plain) + queries = [] + allow(CvtModel).to receive(:tsdb_lookup) { |query, **| queries << query; {} } + @api.get_tlm_values(["INST__LATEST__TEMP1__FORMATTED__LIMITS"], start_time: "2026-09-13T00:00:00Z") + expect(queries).to contain_exactly( + [["INST", "HEALTH_STATUS", "TEMP1", "FORMATTED", "LIMITS"], item("HEALTH_STATUS", "PACKET_TIMESECONDS", "RAW")], + [["INST", "PARAMS", "TEMP1", "RAW", nil], item("PARAMS", "PACKET_TIMESECONDS", "RAW")], + ) + end + it "authorizes every candidate packet" do allow(CvtModel).to receive(:tsdb_lookup).and_return({}) expect(@api).to receive(:authorize).with(hash_including(packet_name: "HEALTH_STATUS")) diff --git a/openc3/test/integration/tsdb/python/test_tlm_api_latest_tsdb.py b/openc3/test/integration/tsdb/python/test_tlm_api_latest_tsdb.py index 859b4aeacc..e31d3f2b84 100644 --- a/openc3/test/integration/tsdb/python/test_tlm_api_latest_tsdb.py +++ b/openc3/test/integration/tsdb/python/test_tlm_api_latest_tsdb.py @@ -48,10 +48,13 @@ TARGET = "PY_LT" PACKET_DEF = {"items": [{"name": "VALUE", "data_type": "INT", "bit_size": 32}]} +# Only PKT_B has units so only it has a formatted (VALUE__F) column +UNITS_PACKET_DEF = {"items": [{"name": "VALUE", "data_type": "INT", "bit_size": 32, "units": "degC"}]} +PACKET_DEFS = {"PKT_A": PACKET_DEF, "PKT_B": UNITS_PACKET_DEF, "REGULAR": PACKET_DEF} # Timeline in ms from the base time (VALUE written to each packet) ROWS = { "PKT_A": [(0, 1), (2000, 3)], - "PKT_B": [(1000, 2), (3000, 4)], + "PKT_B": [(1000, 2), (3000, 4)], # Formatted as '2 degC' and '4 degC' "REGULAR": [(500, 10), (1500, 20), (2500, 30), (3500, 40)], } @@ -68,17 +71,19 @@ def base_ns(self, questdb_client, clean_table, wait_for_data): for packet_name, rows in ROWS.items(): table_name, _ = QuestDBClient.sanitize_table_name(TARGET, packet_name) clean_table(table_name) - questdb_client.create_table(TARGET, packet_name, PACKET_DEF) + questdb_client.create_table(TARGET, packet_name, PACKET_DEFS[packet_name]) for i, (offset_ms, value) in enumerate(rows): columns = questdb_client.process_json_data({"VALUE": value}, table_name) columns["RECEIVED_COUNT"] = i + if packet_name == "PKT_B": + columns["VALUE__F"] = f"{value} degC" questdb_client.write_row(table_name, columns, base_ns + offset_ms * 1_000_000) questdb_client.flush() for packet_name, rows in ROWS.items(): table_name, _ = QuestDBClient.sanitize_table_name(TARGET, packet_name) assert wait_for_data(questdb_client, table_name, len(rows)) >= len(rows) with ( - patch.object(TargetModel, "packet", return_value=PACKET_DEF), + patch.object(TargetModel, "packet", side_effect=lambda _tgt, packet_name, **_kw: PACKET_DEFS[packet_name]), patch.object(TargetModel, "get_item_to_packet_map", return_value={"VALUE": ["PKT_A", "PKT_B"]}), patch("openc3.api.tlm_api.authorize"), ): @@ -95,6 +100,21 @@ def test_returns_a_packet_received_after_the_last_row_of_the_first_candidate(sel # PKT_A was last received at 2000 but PKT_B at 3000 so PKT_B is the latest at 3500 assert get_tlm_values([f"{TARGET}__LATEST__VALUE__RAW"], start_time=self.at(base_ns, 3500)) == [[4, None]] + def test_returns_formatted_from_a_candidate_with_a_formatted_value(self, base_ns): + vals = get_tlm_values([f"{TARGET}__LATEST__VALUE__FORMATTED"], start_time=self.at(base_ns, 3500)) + assert vals == [["4 degC", None]] + + def test_returns_raw_from_a_candidate_without_a_formatted_value(self, base_ns): + assert get_tlm_values([f"{TARGET}__LATEST__VALUE__FORMATTED"], start_time=self.at(base_ns, 2500)) == [[3, None]] + + def test_returns_formatted_from_each_candidate_as_available_in_a_range(self, base_ns): + vals = get_tlm_values( + [f"{TARGET}__LATEST__VALUE__FORMATTED", f"{TARGET}__REGULAR__VALUE__RAW"], + start_time=self.at(base_ns, 0), + end_time=self.at(base_ns, 4000), + ) + assert [row[0][0] for row in vals] == [1, "2 degC", 3, "4 degC"] + def test_returns_latest_alongside_a_regular_item_at_a_point_in_time(self, base_ns): vals = get_tlm_values( [f"{TARGET}__LATEST__VALUE__RAW", f"{TARGET}__REGULAR__VALUE__RAW"], start_time=self.at(base_ns, 3600) diff --git a/openc3/test/integration/tsdb/ruby/tlm_api_latest_tsdb_spec.rb b/openc3/test/integration/tsdb/ruby/tlm_api_latest_tsdb_spec.rb index a37fb9bd5d..b31c9bc93f 100644 --- a/openc3/test/integration/tsdb/ruby/tlm_api_latest_tsdb_spec.rb +++ b/openc3/test/integration/tsdb/ruby/tlm_api_latest_tsdb_spec.rb @@ -32,15 +32,17 @@ class LatestApi # Timeline in ms from the base time (VALUE written to each packet): # PKT_A: 0 => 1, 2000 => 3 - # PKT_B: 1000 => 2, 3000 => 4 + # PKT_B: 1000 => 2, 3000 => 4 (with units so formatted as '2 degC' and '4 degC') # REGULAR: 500 => 10, 1500 => 20, 2500 => 30, 3500 => 40 before(:all) do @test_params = write_multi_table_data({ 'tables' => [ { 'target' => 'RUBY_LT', 'packet' => 'PKT_A', 'data_type' => 'INT', 'bit_size' => 32, 'rows' => [{ 'offset_ms' => 0, 'values' => { 'VALUE' => 1 } }, { 'offset_ms' => 2000, 'values' => { 'VALUE' => 3 } }] }, - { 'target' => 'RUBY_LT', 'packet' => 'PKT_B', 'data_type' => 'INT', 'bit_size' => 32, - 'rows' => [{ 'offset_ms' => 1000, 'values' => { 'VALUE' => 2 } }, { 'offset_ms' => 3000, 'values' => { 'VALUE' => 4 } }] }, + # Only PKT_B has units so only it has a formatted (VALUE__F) column + { 'target' => 'RUBY_LT', 'packet' => 'PKT_B', 'data_type' => 'INT', 'bit_size' => 32, 'units' => 'degC', + 'rows' => [{ 'offset_ms' => 1000, 'values' => { 'VALUE' => 2 }, 'formatted_values' => { 'VALUE' => '2 degC' } }, + { 'offset_ms' => 3000, 'values' => { 'VALUE' => 4 }, 'formatted_values' => { 'VALUE' => '4 degC' } }] }, { 'target' => 'RUBY_LT', 'packet' => 'REGULAR', 'data_type' => 'INT', 'bit_size' => 32, 'rows' => [500, 1500, 2500, 3500].each_with_index.map { |offset, i| { 'offset_ms' => offset, 'values' => { 'VALUE' => (i + 1) * 10 } } } }, ] @@ -75,6 +77,20 @@ def time_at(offset_ms) expect(@api.get_tlm_values(['RUBY_LT__LATEST__VALUE__RAW'], start_time: time_at(3500))).to eq([[4, nil]]) end + it 'returns FORMATTED from a candidate with a formatted value' do + expect(@api.get_tlm_values(['RUBY_LT__LATEST__VALUE__FORMATTED'], start_time: time_at(3500))).to eq([['4 degC', nil]]) + end + + it 'returns RAW from a candidate without a formatted value' do + expect(@api.get_tlm_values(['RUBY_LT__LATEST__VALUE__FORMATTED'], start_time: time_at(2500))).to eq([[3, nil]]) + end + + it 'returns FORMATTED from each candidate as available in a range' do + vals = @api.get_tlm_values(['RUBY_LT__LATEST__VALUE__FORMATTED', 'RUBY_LT__REGULAR__VALUE__RAW'], + start_time: time_at(0), end_time: time_at(4000)) + expect(vals.map { |row| row[0][0] }).to eq([1, '2 degC', 3, '4 degC']) + end + it 'returns LATEST alongside a regular item at a point in time' do vals = @api.get_tlm_values(['RUBY_LT__LATEST__VALUE__RAW', 'RUBY_LT__REGULAR__VALUE__RAW'], start_time: time_at(3600)) expect(vals).to eq([[4, nil], [40, nil]]) From c51b82893132fdf181d71f11c41a8c9ce2de16d3 Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Mon, 12 Oct 2026 17:29:09 -0600 Subject: [PATCH 06/14] fix(telemetry): reject LATEST where it can't name a packet - Streaming rejects LATEST for commands since the item to packet map only covers telemetry - Streaming skips LATEST packets the user can't access instead of failing the whole request, raising only if none are accessible - get_tlm, get_tlm_packet, get_tlm_buffer, get_tlm_cnt, get_packet_derived_items and wait_packet raise a clear LATEST error Co-Authored-By: Claude Opus 5.5 --- .../app/models/streaming_api.rb | 10 ++++++++++ .../spec/models/streaming_api_spec.rb | 16 ++++++++++++++++ openc3/lib/openc3/api/tlm_api.rb | 2 ++ openc3/lib/openc3/script/api_shared.rb | 2 ++ openc3/python/openc3/api/tlm_api.py | 3 +++ openc3/python/openc3/script/api_shared.py | 3 +++ openc3/python/test/api/test_tlm_api.py | 11 ++++++++++- openc3/python/test/script/test_api_shared.py | 4 ++++ openc3/spec/api/tlm_api_spec.rb | 11 ++++++++++- openc3/spec/script/api_shared_spec.rb | 4 ++++ 10 files changed, 64 insertions(+), 2 deletions(-) diff --git a/openc3-cosmos-cmd-tlm-api/app/models/streaming_api.rb b/openc3-cosmos-cmd-tlm-api/app/models/streaming_api.rb index 7b059e50ee..cc5218d4dc 100644 --- a/openc3-cosmos-cmd-tlm-api/app/models/streaming_api.rb +++ b/openc3-cosmos-cmd-tlm-api/app/models/streaming_api.rb @@ -45,13 +45,23 @@ def build_item_collection(collection, data, start_time: nil, end_time: nil, scop # Unwrap LATEST packet into individual items parsed = StreamingKey.parse(key, item_key: true) if parsed.packet_name == 'LATEST' + # The item to packet map only covers telemetry + raise "LATEST is only supported for telemetry: #{key}" if parsed.cmd_or_tlm == :CMD item_map = OpenC3::TargetModel.get_item_to_packet_map(parsed.target_name, scope: scope) packet_names = item_map[parsed.item_name] raise "Item '#{parsed.target_name} LATEST #{parsed.item_name}' does not exist for scope: #{scope}" unless packet_names + # Skip packets the user can't access unless they can't access any of them + auth_error = nil + added = false packet_names.each do |packet_name| new_key = parsed.with(packet_name: packet_name).to_key_string collection.add(StreamingObject.new(new_key, start_time, end_time, item_key: item_key, scope: scope, token: token)) + added = true + rescue OpenC3::AuthError, OpenC3::ForbiddenError => e + OpenC3::Logger.info("Skipping unauthorized LATEST packet: #{new_key}") + auth_error = e end + raise auth_error if auth_error and not added else collection.add(StreamingObject.new(key, start_time, end_time, item_key: item_key, scope: scope, token: token)) end diff --git a/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb b/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb index 9c463f2c6d..7ec9da16d9 100644 --- a/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb +++ b/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb @@ -541,6 +541,22 @@ def build(items) expect(collection.length).to eq(2) end + it 'rejects LATEST for commands' do + expect { build(['DECOM__CMD__INST__LATEST__TEMP1__CONVERTED']) }.to raise_error(/LATEST is only supported for telemetry/) + end + + it 'skips LATEST packets the user cannot access' do + allow_any_instance_of(StreamingObject).to receive(:authorize) do |_object, packet_name:, **| + raise OpenC3::ForbiddenError, 'forbidden' if packet_name == 'PARAMS' + end + expect(build([latest_key]).objects.map(&:packet_name)).to eq(['HEALTH_STATUS']) + end + + it 'raises when the user cannot access any LATEST packet' do + allow_any_instance_of(StreamingObject).to receive(:authorize).and_raise(OpenC3::ForbiddenError, 'forbidden') + expect { build([latest_key]) }.to raise_error(OpenC3::ForbiddenError) + end + it 'removes an item added with an item_key when removed by key alone' do collection = build([[explicit_key, '0']]) build([explicit_key]).objects.each { |o| collection.remove(o) } diff --git a/openc3/lib/openc3/api/tlm_api.rb b/openc3/lib/openc3/api/tlm_api.rb index 71fe6e0c33..c78f480344 100644 --- a/openc3/lib/openc3/api/tlm_api.rb +++ b/openc3/lib/openc3/api/tlm_api.rb @@ -558,6 +558,8 @@ def _extract_target_packet_names(method_name, *args) if target_name.nil? or packet_name.nil? raise "ERROR: Both target name and packet name required. Usage: #{method_name}(\"TGT PKT\") or #{method_name}(\"TGT\", \"PKT\")" end + # LATEST picks a packet per item so it can't identify a whole packet + raise "ERROR: LATEST is not supported by #{method_name}(), specify a packet name" if packet_name == 'LATEST' return [target_name, packet_name] end diff --git a/openc3/lib/openc3/script/api_shared.rb b/openc3/lib/openc3/script/api_shared.rb index 6e3f86136a..93ea24cfff 100644 --- a/openc3/lib/openc3/script/api_shared.rb +++ b/openc3/lib/openc3/script/api_shared.rb @@ -596,6 +596,8 @@ def _wait_packet(check, quiet: false, scope: $openc3_scope, token: $openc3_token) type = (check ? 'CHECK' : 'WAIT') + # LATEST would compare the RECEIVED_COUNT of whichever packet was received most recently + raise "LATEST is not supported by wait_packet, specify a packet name" if packet_name.to_s.upcase == 'LATEST' initial_count = tlm(target_name, packet_name, 'RECEIVED_COUNT', scope: scope, token: token) # If the packet has not been received the initial_count could be nil initial_count = 0 unless initial_count diff --git a/openc3/python/openc3/api/tlm_api.py b/openc3/python/openc3/api/tlm_api.py index f09eff1c05..934932a4c7 100644 --- a/openc3/python/openc3/api/tlm_api.py +++ b/openc3/python/openc3/api/tlm_api.py @@ -686,6 +686,9 @@ def _extract_target_packet_names(method_name, *args): raise RuntimeError( f'ERROR: Both target name and packet name required. Usage: {method_name}("TGT PKT") or {method_name}("TGT", "PKT")' ) + # LATEST picks a packet per item so it can't identify a whole packet + if packet_name == "LATEST": + raise RuntimeError(f"ERROR: LATEST is not supported by {method_name}(), specify a packet name") return target_name, packet_name diff --git a/openc3/python/openc3/script/api_shared.py b/openc3/python/openc3/script/api_shared.py index 7919d5ea37..2b17fe8d5d 100644 --- a/openc3/python/openc3/script/api_shared.py +++ b/openc3/python/openc3/script/api_shared.py @@ -695,6 +695,9 @@ def _wait_packet( type = "CHECK" else: type = "WAIT" + # LATEST would compare the RECEIVED_COUNT of whichever packet was received most recently + if str(packet_name).upper() == "LATEST": + raise RuntimeError("LATEST is not supported by wait_packet, specify a packet name") initial_count = openc3.script.API_SERVER.tlm(target_name, packet_name, "RECEIVED_COUNT", scope=scope) # If the packet has not been received the initial_count could be None if initial_count is None: diff --git a/openc3/python/test/api/test_tlm_api.py b/openc3/python/test/api/test_tlm_api.py index 4ebc2c025a..f1f2478c18 100644 --- a/openc3/python/test/api/test_tlm_api.py +++ b/openc3/python/test/api/test_tlm_api.py @@ -684,7 +684,7 @@ def test_complains_about_non_existant_packets(self): get_tlm_packet("INST BLAH") def test_complains_using_latest(self): - with self.assertRaisesRegex(RuntimeError, "Packet definition 'INST LATEST' does not exist"): + with self.assertRaisesRegex(RuntimeError, "LATEST is not supported by get_tlm_packet"): get_tlm_packet("INST", "LATEST") def test_complains_about_non_existant_value_types(self): @@ -989,6 +989,15 @@ def test_get_tlm_values_historical_latest_authorizes_every_candidate(self, mock_ [call.kwargs["packet_name"] for call in mock_authorize.call_args_list], ["HEALTH_STATUS", "PARAMS"] ) + def test_packet_apis_complain_that_latest_does_not_identify_a_packet(self): + for method in [get_tlm_buffer, get_tlm_packet, get_tlm, get_tlm_cnt, get_packet_derived_items]: + with self.assertRaisesRegex( + RuntimeError, f"LATEST is not supported by {method.__name__}\\(\\), specify a packet name" + ): + method("INST", "LATEST") + with self.assertRaisesRegex(RuntimeError, "LATEST is not supported"): + method("inst latest") + def test_get_tlm_values_accepts_the_limits_suffix_from_the_cvt(self): vals = get_tlm_values(["INST__HEALTH_STATUS__TEMP1__CONVERTED__LIMITS"]) self.assertEqual(vals[0][0], (-100.0)) diff --git a/openc3/python/test/script/test_api_shared.py b/openc3/python/test/script/test_api_shared.py index 45ed548989..81a95ae8fd 100644 --- a/openc3/python/test/script/test_api_shared.py +++ b/openc3/python/test/script/test_api_shared.py @@ -1040,6 +1040,10 @@ def test_waits_and_checks_a_logical_expression(self): ): wait_check_expression("'STRING' == STRING", 0.01) + def test_wait_packet_complains_about_latest(self): + with self.assertRaisesRegex(RuntimeError, "LATEST is not supported by wait_packet"): + wait_packet("INST", "latest", 1, 0.1) + def test_wait_packet_prints_warning_if_packet_not_received(self): global count count = False diff --git a/openc3/spec/api/tlm_api_spec.rb b/openc3/spec/api/tlm_api_spec.rb index c7a28b29c1..5caf514a95 100644 --- a/openc3/spec/api/tlm_api_spec.rb +++ b/openc3/spec/api/tlm_api_spec.rb @@ -559,6 +559,15 @@ def test_tlm_unknown(method) end end + describe "packet APIs with LATEST" do + it "complains that LATEST does not identify a packet" do + %w(get_tlm_buffer get_tlm_packet get_tlm get_tlm_cnt get_packet_derived_items).each do |method_name| + expect { @api.public_send(method_name, "INST", "LATEST") }.to raise_error(RuntimeError, "ERROR: LATEST is not supported by #{method_name}(), specify a packet name") + expect { @api.public_send(method_name, "inst latest") }.to raise_error(RuntimeError, /LATEST is not supported/) + end + end + end + describe "get_tlm_packet" do it "complains about non-existent targets" do expect { @api.get_tlm_packet("BLAH", "HEALTH_STATUS") }.to raise_error(RuntimeError, "Packet definition 'BLAH HEALTH_STATUS' does not exist") @@ -569,7 +578,7 @@ def test_tlm_unknown(method) end it "complains using LATEST" do - expect { @api.get_tlm_packet("INST", "LATEST") }.to raise_error(RuntimeError, "Packet definition 'INST LATEST' does not exist") + expect { @api.get_tlm_packet("INST", "LATEST") }.to raise_error(RuntimeError, "ERROR: LATEST is not supported by get_tlm_packet(), specify a packet name") end it "complains about non-existent value_types" do diff --git a/openc3/spec/script/api_shared_spec.rb b/openc3/spec/script/api_shared_spec.rb index 1f974362b7..2719ab3234 100644 --- a/openc3/spec/script/api_shared_spec.rb +++ b/openc3/spec/script/api_shared_spec.rb @@ -860,6 +860,10 @@ def openc3_script_sleep(_sleep_time = nil) @sleep_cancel = cancel end + it "complains about LATEST" do + expect { wait_packet("INST", "latest", 1, 0.1) }.to raise_error(/LATEST is not supported by wait_packet/) + end + it "prints warning if packet not received" do @count = false capture_io do |stdout| From e4e1642452a18382254db02aca882fe9e571d966 Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Mon, 12 Oct 2026 17:55:46 -0600 Subject: [PATCH 07/14] fix(telemetry): reject LATEST in subscribe_packets and inject_tlm - Both take a packet name directly rather than through _extract_target_packet_names so they missed the LATEST check; share it as _tlm_reject_latest (Ruby and Python) Co-Authored-By: Claude Opus 5.5 --- openc3/lib/openc3/api/tlm_api.rb | 10 ++++++++-- openc3/python/openc3/api/tlm_api.py | 10 ++++++++-- openc3/python/test/api/test_tlm_api.py | 6 ++++++ openc3/spec/api/tlm_api_spec.rb | 5 +++++ 4 files changed, 27 insertions(+), 4 deletions(-) diff --git a/openc3/lib/openc3/api/tlm_api.rb b/openc3/lib/openc3/api/tlm_api.rb index c78f480344..860a2b52fe 100644 --- a/openc3/lib/openc3/api/tlm_api.rb +++ b/openc3/lib/openc3/api/tlm_api.rb @@ -119,6 +119,7 @@ def set_tlm(*args, type: :CONVERTED, manual: false, cache_timeout: nil, scope: $ # @param type [Symbol] Telemetry type, :RAW, :CONVERTED (default), :FORMATTED # @param received_time [Integer, nil] Optional received time as nanoseconds since Unix epoch def inject_tlm(target_name, packet_name, item_hash = nil, type: :CONVERTED, stored: false, received_time: nil, manual: false, scope: $openc3_scope, token: $openc3_token) + _tlm_reject_latest(packet_name.to_s.upcase, 'inject_tlm') authorize(permission: 'tlm_set', target_name: target_name, packet_name: packet_name, manual: manual, scope: scope, token: token) type = type.to_s.intern target_name = target_name.upcase @@ -453,6 +454,7 @@ def subscribe_packets(packets, manual: false, scope: $openc3_scope, token: $open packets.each do |target_name, packet_name| target_name = target_name.upcase packet_name = packet_name.upcase + _tlm_reject_latest(packet_name, 'subscribe_packets') authorize(permission: 'tlm', target_name: target_name, packet_name: packet_name, manual: manual, scope: scope, token: token) topic = "#{scope}__DECOM__{#{target_name}}__#{packet_name}" db_shard = Store.db_shard_for_target(target_name, scope: scope) @@ -558,11 +560,15 @@ def _extract_target_packet_names(method_name, *args) if target_name.nil? or packet_name.nil? raise "ERROR: Both target name and packet name required. Usage: #{method_name}(\"TGT PKT\") or #{method_name}(\"TGT\", \"PKT\")" end - # LATEST picks a packet per item so it can't identify a whole packet - raise "ERROR: LATEST is not supported by #{method_name}(), specify a packet name" if packet_name == 'LATEST' + _tlm_reject_latest(packet_name, method_name) return [target_name, packet_name] end + # LATEST picks a packet per item so it can't identify a whole packet + def _tlm_reject_latest(packet_name, method_name) + raise "ERROR: LATEST is not supported by #{method_name}(), specify a packet name" if packet_name == 'LATEST' + end + def _extract_target_packet_item_names(method_name, *args) target_name = nil packet_name = nil diff --git a/openc3/python/openc3/api/tlm_api.py b/openc3/python/openc3/api/tlm_api.py index 934932a4c7..1807a7187b 100644 --- a/openc3/python/openc3/api/tlm_api.py +++ b/openc3/python/openc3/api/tlm_api.py @@ -142,6 +142,7 @@ def inject_tlm( scope=OPENC3_SCOPE, received_time=None, ): + _tlm_reject_latest(str(packet_name).upper(), "inject_tlm") authorize( permission="tlm_set", target_name=target_name, @@ -548,6 +549,7 @@ def subscribe_packets(packets, scope=OPENC3_SCOPE): for target_name, packet_name in packets: target_name = target_name.upper() packet_name = packet_name.upper() + _tlm_reject_latest(packet_name, "subscribe_packets") authorize( permission="tlm", target_name=target_name, @@ -686,10 +688,14 @@ def _extract_target_packet_names(method_name, *args): raise RuntimeError( f'ERROR: Both target name and packet name required. Usage: {method_name}("TGT PKT") or {method_name}("TGT", "PKT")' ) - # LATEST picks a packet per item so it can't identify a whole packet + _tlm_reject_latest(packet_name, method_name) + return target_name, packet_name + + +# LATEST picks a packet per item so it can't identify a whole packet +def _tlm_reject_latest(packet_name, method_name): if packet_name == "LATEST": raise RuntimeError(f"ERROR: LATEST is not supported by {method_name}(), specify a packet name") - return target_name, packet_name def _extract_target_packet_item_names(method_name, *args): diff --git a/openc3/python/test/api/test_tlm_api.py b/openc3/python/test/api/test_tlm_api.py index f1f2478c18..6ef0106240 100644 --- a/openc3/python/test/api/test_tlm_api.py +++ b/openc3/python/test/api/test_tlm_api.py @@ -998,6 +998,12 @@ def test_packet_apis_complain_that_latest_does_not_identify_a_packet(self): with self.assertRaisesRegex(RuntimeError, "LATEST is not supported"): method("inst latest") + def test_subscribe_packets_and_inject_tlm_complain_about_latest(self): + with self.assertRaisesRegex(RuntimeError, r"LATEST is not supported by subscribe_packets\(\)"): + subscribe_packets([["INST", "latest"]]) + with self.assertRaisesRegex(RuntimeError, r"LATEST is not supported by inject_tlm\(\)"): + inject_tlm("INST", "latest", {"TEMP1": 10}) + def test_get_tlm_values_accepts_the_limits_suffix_from_the_cvt(self): vals = get_tlm_values(["INST__HEALTH_STATUS__TEMP1__CONVERTED__LIMITS"]) self.assertEqual(vals[0][0], (-100.0)) diff --git a/openc3/spec/api/tlm_api_spec.rb b/openc3/spec/api/tlm_api_spec.rb index 5caf514a95..743652d303 100644 --- a/openc3/spec/api/tlm_api_spec.rb +++ b/openc3/spec/api/tlm_api_spec.rb @@ -566,6 +566,11 @@ def test_tlm_unknown(method) expect { @api.public_send(method_name, "inst latest") }.to raise_error(RuntimeError, /LATEST is not supported/) end end + + it "complains about LATEST in subscribe_packets and inject_tlm" do + expect { @api.subscribe_packets([["INST", "latest"]]) }.to raise_error(RuntimeError, "ERROR: LATEST is not supported by subscribe_packets(), specify a packet name") + expect { @api.inject_tlm("INST", "latest", { TEMP1: 10 }) }.to raise_error(RuntimeError, "ERROR: LATEST is not supported by inject_tlm(), specify a packet name") + end end describe "get_tlm_packet" do From f1b2ac412062ffdb791958f3f52d17bd6cc5b35c Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Tue, 13 Oct 2026 09:15:49 -0600 Subject: [PATCH 08/14] fix(telemetry): return RAW for DERIVED items in python get_tlm_available - Python _tlm_available_value_type counted a DERIVED read conversion as CONVERTED, but Packet.decom writes no __C for it, so historical lookups queried a missing QuestDB column - Add Python and Ruby tests for a DERIVED item with a read conversion Co-Authored-By: Claude Opus 5.5 --- openc3/python/openc3/api/tlm_api.py | 5 ++++- openc3/python/test/api/test_tlm_api.py | 10 ++++++++++ openc3/spec/api/tlm_api_spec.rb | 6 ++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/openc3/python/openc3/api/tlm_api.py b/openc3/python/openc3/api/tlm_api.py index 1807a7187b..ca4523d1cc 100644 --- a/openc3/python/openc3/api/tlm_api.py +++ b/openc3/python/openc3/api/tlm_api.py @@ -806,7 +806,10 @@ def _tlm_available_value_type(item_config, item_name, value_type): # Arrays must be accessed as RAW since there's no conversion if item_config.get("array_size") is not None: return "RAW" - converted = item_config.get("read_conversion") or item_config.get("states") + # This logic must match the logic in Packet.decom + converted = item_config.get("states") or ( + item_config.get("read_conversion") and item_config.get("data_type") != "DERIVED" + ) if value_type == "FORMATTED" or value_type == "WITH_UNITS": if item_config.get("format_string") or item_config.get("units"): return "FORMATTED" diff --git a/openc3/python/test/api/test_tlm_api.py b/openc3/python/test/api/test_tlm_api.py index 6ef0106240..6581ee61d3 100644 --- a/openc3/python/test/api/test_tlm_api.py +++ b/openc3/python/test/api/test_tlm_api.py @@ -1192,6 +1192,16 @@ def test_get_tlm_available_returns_proper_value_types_based_on_item_configuratio # DURATION should return RAW (no conversions) self.assertEqual(result[3], "INST__HEALTH_STATUS__DURATION__RAW") + def test_get_tlm_available_returns_raw_for_derived_items_with_a_read_conversion(self): + # Packet.decom stores a DERIVED item's read conversion as the raw value with no __C + items = [ + "INST__HEALTH_STATUS__DERIVED_GENERIC__CONVERTED", + "INST__HEALTH_STATUS__DERIVED_GENERIC__FORMATTED", + ] + result = get_tlm_available(items) + self.assertEqual(result[0], "INST__HEALTH_STATUS__DERIVED_GENERIC__RAW") + self.assertEqual(result[1], "INST__HEALTH_STATUS__DERIVED_GENERIC__RAW") + def test_get_tlm_available_handles_fallback_value_types(self): # Test fallback logic: WITH_UNITS -> FORMATTED -> CONVERTED -> RAW items = [ diff --git a/openc3/spec/api/tlm_api_spec.rb b/openc3/spec/api/tlm_api_spec.rb index 743652d303..331e3aec94 100644 --- a/openc3/spec/api/tlm_api_spec.rb +++ b/openc3/spec/api/tlm_api_spec.rb @@ -722,6 +722,12 @@ def test_tlm_unknown(method) expect(@api.get_tlm_available(['INST__LATEST__TEMP1__FORMATTED'])).to eql(['INST__LATEST__TEMP1__FORMATTED__LIMITS']) end + it "returns RAW for DERIVED items with a read conversion" do + # Packet#decom stores a DERIVED item's read conversion as the raw value with no __C + items = ['INST__HEALTH_STATUS__TEMP1HIGH__CONVERTED', 'INST__HEALTH_STATUS__TEMP1HIGH__FORMATTED'] + expect(@api.get_tlm_available(items)).to eql(['INST__HEALTH_STATUS__TEMP1HIGH__RAW', 'INST__HEALTH_STATUS__TEMP1HIGH__RAW']) + end + it "returns a valid list of items based on inputs" do items = [] # Ask for WITH_UNITS for an item which has various formats From 0733dd3a5b074310c824118f541a534f487bf329 Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Thu, 15 Oct 2026 15:05:48 -0600 Subject: [PATCH 09/14] fix(telemetry): address LATEST review findings - Match streaming removal fallback on source_key so removing an explicit item by key leaves a LATEST item for the same packet - Authorize every LATEST candidate packet in get_tlm_available before reading its definition, in Ruby and Python - Upcase target_name before building the item-to-packet map key so lowercase lookups share the refreshed cache and store entry - Align test_tlm_api_latest_tsdb base timestamp to microseconds Co-Authored-By: Claude Opus 5.5 --- .../app/models/streaming_api.rb | 2 +- .../app/models/streaming_object.rb | 5 ++++- .../app/models/streaming_object_collection.rb | 6 ++++-- .../spec/models/streaming_api_spec.rb | 13 +++++++++++++ openc3/lib/openc3/api/tlm_api.rb | 5 ++++- openc3/lib/openc3/models/target_model.rb | 4 +++- openc3/python/openc3/api/tlm_api.py | 14 ++++++++------ openc3/python/openc3/models/target_model.py | 4 +++- openc3/python/test/api/test_tlm_api.py | 9 +++++++++ openc3/python/test/models/test_target_model.py | 5 +++++ openc3/spec/api/tlm_api_spec.rb | 10 ++++++++++ openc3/spec/models/target_model_spec.rb | 7 +++++++ .../tsdb/python/test_tlm_api_latest_tsdb.py | 6 +++++- 13 files changed, 76 insertions(+), 14 deletions(-) diff --git a/openc3-cosmos-cmd-tlm-api/app/models/streaming_api.rb b/openc3-cosmos-cmd-tlm-api/app/models/streaming_api.rb index cc5218d4dc..5b730c24ed 100644 --- a/openc3-cosmos-cmd-tlm-api/app/models/streaming_api.rb +++ b/openc3-cosmos-cmd-tlm-api/app/models/streaming_api.rb @@ -55,7 +55,7 @@ def build_item_collection(collection, data, start_time: nil, end_time: nil, scop added = false packet_names.each do |packet_name| new_key = parsed.with(packet_name: packet_name).to_key_string - collection.add(StreamingObject.new(new_key, start_time, end_time, item_key: item_key, scope: scope, token: token)) + collection.add(StreamingObject.new(new_key, start_time, end_time, item_key: item_key, source_key: key, scope: scope, token: token)) added = true rescue OpenC3::AuthError, OpenC3::ForbiddenError => e OpenC3::Logger.info("Skipping unauthorized LATEST packet: #{new_key}") diff --git a/openc3-cosmos-cmd-tlm-api/app/models/streaming_object.rb b/openc3-cosmos-cmd-tlm-api/app/models/streaming_object.rb index 8c92337111..31305f8baf 100644 --- a/openc3-cosmos-cmd-tlm-api/app/models/streaming_object.rb +++ b/openc3-cosmos-cmd-tlm-api/app/models/streaming_object.rb @@ -40,11 +40,14 @@ class StreamingObject attr_reader :id attr_reader :realtime attr_reader :item_key + # The key as subscribed, which differs from key when a LATEST item expands to a concrete packet + attr_reader :source_key - def initialize(key, start_time_nsec, end_time_nsec, item_key: nil, scope:, token: nil) + def initialize(key, start_time_nsec, end_time_nsec, item_key: nil, source_key: nil, scope:, token: nil) key = key.upcase @key = key @item_key = item_key + @source_key = source_key ? source_key.upcase : key parsed = StreamingKey.parse(key, item_key: !!item_key) @stream_mode = parsed.stream_mode @cmd_or_tlm = parsed.cmd_or_tlm diff --git a/openc3-cosmos-cmd-tlm-api/app/models/streaming_object_collection.rb b/openc3-cosmos-cmd-tlm-api/app/models/streaming_object_collection.rb index 890cf03906..92b6ecb6e4 100644 --- a/openc3-cosmos-cmd-tlm-api/app/models/streaming_object_collection.rb +++ b/openc3-cosmos-cmd-tlm-api/app/models/streaming_object_collection.rb @@ -62,8 +62,10 @@ def remove(object) _remove_found(found_object) elsif object.item_key # Items can be added as [key, item_key] pairs but removed by key alone, - # so fall back to removing every item object with a matching key - @objects.select { |obj| obj.item_key and obj.key == object.key }.each do |obj| + # so fall back to removing every item object subscribed with the same key. + # Match on source_key so removing an explicit item doesn't also remove a + # LATEST subscription which expanded to the same concrete key (and vice versa) + @objects.select { |obj| obj.item_key and obj.source_key == object.source_key }.each do |obj| _remove_found(obj) end end diff --git a/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb b/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb index 7ec9da16d9..e85f80e091 100644 --- a/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb +++ b/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb @@ -562,6 +562,19 @@ def build(items) build([explicit_key]).objects.each { |o| collection.remove(o) } expect(collection).to be_empty end + + it 'removing an explicit item by key alone leaves the LATEST item' do + collection = build([latest_key, [explicit_key, '0']]) + build([explicit_key]).objects.each { |o| collection.remove(o) } + expect(collection.objects.map(&:item_key).uniq).to eq([latest_key]) + expect(collection.length).to eq(2) + end + + it 'removing a LATEST item by key alone leaves the explicit item' do + collection = build([[latest_key, '0'], explicit_key]) + build([latest_key]).objects.each { |o| collection.remove(o) } + expect(collection.objects.map { |o| [o.key, o.item_key] }).to eq([[explicit_key, explicit_key]]) + end end describe 'add with COSMOS_ALL packets and authorization' do diff --git a/openc3/lib/openc3/api/tlm_api.rb b/openc3/lib/openc3/api/tlm_api.rb index 860a2b52fe..3371276679 100644 --- a/openc3/lib/openc3/api/tlm_api.rb +++ b/openc3/lib/openc3/api/tlm_api.rb @@ -285,7 +285,10 @@ def get_tlm_available(items, manual: false, scope: $openc3_scope, token: $openc3 else packet_names = [packet_name] end - authorize(permission: 'tlm', target_name: target_name, packet_name: packet_name, scope: scope, token: token) + # Authorize every packet whose definition is read below, not just the latest one + (packet_names | [packet_name]).each do |name| + authorize(permission: 'tlm', target_name: target_name, packet_name: name, scope: scope, token: token) + end begin packet_items = packet_names.map { |name| TargetModel.packet_item(target_name, name, item_name, scope: scope) } available_type = packet_items.map { |packet_item| _tlm_available_value_type(packet_item, item_name, value_type) }.max_by { |type| VALUE_TYPE_ORDER.index(type) } diff --git a/openc3/lib/openc3/models/target_model.rb b/openc3/lib/openc3/models/target_model.rb index a5c7e5e73d..7411d8429e 100644 --- a/openc3/lib/openc3/models/target_model.rb +++ b/openc3/lib/openc3/models/target_model.rb @@ -429,11 +429,13 @@ def self.limits_groups(scope:) end def self.get_item_to_packet_map(target_name, scope:) + # Normalize before building the key so lowercase lookups share the entry + # that update_store_item_map and undeploy refresh and invalidate + target_name = target_name.upcase # The cache is keyed by item_map_key since the same target can exist in multiple scopes item_map_key = "#{scope}__#{target_name}__item_to_packet_map" cache_time, item_map = @@item_map_cache[item_map_key] return item_map if item_map and (Time.now - cache_time) < ITEM_MAP_CACHE_TIMEOUT - target_name = target_name.upcase store = store_for_target(target_name, scope: scope) json_data = store.get(item_map_key) if json_data diff --git a/openc3/python/openc3/api/tlm_api.py b/openc3/python/openc3/api/tlm_api.py index ca4523d1cc..134ecd09b3 100644 --- a/openc3/python/openc3/api/tlm_api.py +++ b/openc3/python/openc3/api/tlm_api.py @@ -337,12 +337,14 @@ def get_tlm_available(items, manual=False, scope=OPENC3_SCOPE): else: packet_names = [packet_name] - authorize( - permission="tlm", - target_name=target_name, - packet_name=packet_name, - scope=scope, - ) + # Authorize every packet whose definition is read below, not just the latest one + for name in dict.fromkeys([*packet_names, packet_name]): + authorize( + permission="tlm", + target_name=target_name, + packet_name=name, + scope=scope, + ) try: item_configs = [TargetModel.packet_item(target_name, name, item_name, scope=scope) for name in packet_names] diff --git a/openc3/python/openc3/models/target_model.py b/openc3/python/openc3/models/target_model.py index 88d2985479..62e24ac3f5 100644 --- a/openc3/python/openc3/models/target_model.py +++ b/openc3/python/openc3/models/target_model.py @@ -228,13 +228,15 @@ def limits_groups(cls, scope: str = OPENC3_SCOPE): @classmethod def get_item_to_packet_map(cls, target_name: str, scope: str = OPENC3_SCOPE): + # Normalize before building the key so lowercase lookups share the entry + # that update_store_item_map refreshes + target_name = target_name.upper() # The cache is keyed by item_map_key since the same target can exist in multiple scopes item_map_key = f"{scope}__{target_name}__item_to_packet_map" if item_map_key in TargetModel.item_map_cache: cache_time, item_map = TargetModel.item_map_cache[item_map_key] if (time.time() - cache_time) < TargetModel.ITEM_MAP_CACHE_TIMEOUT: return item_map - target_name = target_name.upper() store = cls._store_for_target(target_name, scope) json_data = store.get(item_map_key) if json_data: diff --git a/openc3/python/test/api/test_tlm_api.py b/openc3/python/test/api/test_tlm_api.py index 6581ee61d3..6c4ebf7e32 100644 --- a/openc3/python/test/api/test_tlm_api.py +++ b/openc3/python/test/api/test_tlm_api.py @@ -980,6 +980,15 @@ def test_get_tlm_available_returns_the_best_type_and_limits_of_any_latest_packet get_tlm_available(["INST__LATEST__TEMP1__FORMATTED"]), ["INST__LATEST__TEMP1__FORMATTED__LIMITS"] ) + @latest_candidates() + @patch("openc3.api.tlm_api.authorize") + def test_get_tlm_available_latest_authorizes_every_candidate(self, mock_authorize): + with patch.object(CvtModel, "determine_latest_packet_for_item", return_value="PARAMS"): + get_tlm_available(["INST__LATEST__TEMP1__CONVERTED"]) + self.assertCountEqual( + [call.kwargs["packet_name"] for call in mock_authorize.call_args_list], ["HEALTH_STATUS", "PARAMS"] + ) + @latest_candidates() @patch("openc3.api.tlm_api.authorize") def test_get_tlm_values_historical_latest_authorizes_every_candidate(self, mock_authorize): diff --git a/openc3/python/test/models/test_target_model.py b/openc3/python/test/models/test_target_model.py index 5d9a6f1bca..08779f9ce9 100644 --- a/openc3/python/test/models/test_target_model.py +++ b/openc3/python/test/models/test_target_model.py @@ -149,6 +149,11 @@ def test_gets_item_to_packet_map_from_cache(self): TargetModel.item_map_cache["DEFAULT__INST__item_to_packet_map"][0], ) + def test_shares_the_item_to_packet_map_cache_entry_for_a_lowercase_target(self): + TargetModel.get_item_to_packet_map("inst", scope="DEFAULT") + self.assertIn("DEFAULT__INST__item_to_packet_map", TargetModel.item_map_cache) + self.assertNotIn("DEFAULT__inst__item_to_packet_map", TargetModel.item_map_cache) + def test_caches_the_item_to_packet_map_per_scope(self): TargetModel.get_item_to_packet_map("INST", scope="DEFAULT") Store.set("OTHER__INST__item_to_packet_map", json.dumps({"OTHER_ITEM": ["OTHER_PKT"]})) diff --git a/openc3/spec/api/tlm_api_spec.rb b/openc3/spec/api/tlm_api_spec.rb index 331e3aec94..7d16e9d407 100644 --- a/openc3/spec/api/tlm_api_spec.rb +++ b/openc3/spec/api/tlm_api_spec.rb @@ -722,6 +722,16 @@ def test_tlm_unknown(method) expect(@api.get_tlm_available(['INST__LATEST__TEMP1__FORMATTED'])).to eql(['INST__LATEST__TEMP1__FORMATTED__LIMITS']) end + it "authorizes every LATEST packet before reading its definition" do + allow(TargetModel).to receive(:get_item_to_packet_map).and_return({ 'TEMP1' => ['PARAMS', 'HEALTH_STATUS'] }) + allow(CvtModel).to receive(:determine_latest_packet_for_item).and_return('PARAMS') + allow(@api).to receive(:authorize) do |packet_name:, **| + raise ForbiddenError, 'forbidden' if packet_name == 'HEALTH_STATUS' + end + expect(TargetModel).not_to receive(:packet_item) + expect { @api.get_tlm_available(['INST__LATEST__TEMP1__FORMATTED']) }.to raise_error(ForbiddenError) + end + it "returns RAW for DERIVED items with a read conversion" do # Packet#decom stores a DERIVED item's read conversion as the raw value with no __C items = ['INST__HEALTH_STATUS__TEMP1HIGH__CONVERTED', 'INST__HEALTH_STATUS__TEMP1HIGH__FORMATTED'] diff --git a/openc3/spec/models/target_model_spec.rb b/openc3/spec/models/target_model_spec.rb index da750fe6c2..2b3c7da180 100644 --- a/openc3/spec/models/target_model_spec.rb +++ b/openc3/spec/models/target_model_spec.rb @@ -465,6 +465,13 @@ def shadow_response(body) expect(cache["DEFAULT__INST__item_to_packet_map"][0]).to eq(orig_cache["DEFAULT__INST__item_to_packet_map"][0]) end + it "shares the item-to-packet map cache entry for a lowercase target" do + TargetModel.get_item_to_packet_map("inst", scope: "DEFAULT") + cache = TargetModel.class_variable_get(:@@item_map_cache) + expect(cache.keys).to include("DEFAULT__INST__item_to_packet_map") + expect(cache.keys).not_to include("DEFAULT__inst__item_to_packet_map") + end + it "caches the item-to-packet map per scope" do TargetModel.get_item_to_packet_map("INST", scope: "DEFAULT") Store.set("OTHER__INST__item_to_packet_map", JSON.generate({ "OTHER_ITEM" => ["OTHER_PKT"] })) diff --git a/openc3/test/integration/tsdb/python/test_tlm_api_latest_tsdb.py b/openc3/test/integration/tsdb/python/test_tlm_api_latest_tsdb.py index e31d3f2b84..a9e08a2ce8 100644 --- a/openc3/test/integration/tsdb/python/test_tlm_api_latest_tsdb.py +++ b/openc3/test/integration/tsdb/python/test_tlm_api_latest_tsdb.py @@ -30,6 +30,7 @@ import pytest + # Add openc3 python path for imports # Path: openc3/test/integration/tsdb/python -> openc3/python sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "..", "..", "..", "python")) @@ -42,10 +43,12 @@ os.environ.setdefault("OPENC3_SCOPE", "DEFAULT") from conftest import requires_questdb + from openc3.api.tlm_api import get_tlm_values from openc3.models.target_model import TargetModel from openc3.utilities.questdb_client import QuestDBClient + TARGET = "PY_LT" PACKET_DEF = {"items": [{"name": "VALUE", "data_type": "INT", "bit_size": 32}]} # Only PKT_B has units so only it has a formatted (VALUE__F) column @@ -67,7 +70,8 @@ def ts_to_iso(ts_ns): class TestTlmApiLatestTsdb: @pytest.fixture def base_ns(self, questdb_client, clean_table, wait_for_data): - base_ns = int(time.time() * 1e9) + # Align to microseconds since ts_to_iso drops nanoseconds and could round the range start past the first row + base_ns = time.time_ns() // 1000 * 1000 for packet_name, rows in ROWS.items(): table_name, _ = QuestDBClient.sanitize_table_name(TARGET, packet_name) clean_table(table_name) From 2c83b1bc020f2a8ac5dd1cf448936dea15a31e32 Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Thu, 15 Oct 2026 16:08:45 -0600 Subject: [PATCH 10/14] fix(telemetry): address coderabbit LATEST review findings Key-only streaming removal now drops every item_key for the key, and the item subscription ID includes source_key so LATEST and explicit items can coexist. Historical LATEST lookups skip candidate packets with no QuestDB table yet. Co-Authored-By: Claude Opus 5.5 --- .../app/models/streaming_object.rb | 6 +- .../app/models/streaming_object_collection.rb | 15 ++--- .../spec/models/streaming_api_spec.rb | 20 ++++++ openc3/lib/openc3/api/tlm_api.rb | 28 ++++++--- openc3/lib/openc3/models/cvt_model.rb | 5 ++ openc3/lib/openc3/utilities/questdb_client.rb | 17 ++++++ openc3/python/openc3/api/tlm_api.py | 42 ++++++++----- openc3/python/openc3/models/cvt_model.py | 5 ++ .../python/openc3/utilities/questdb_client.py | 30 +++++++++ openc3/python/test/api/test_limits_api.py | 2 + openc3/python/test/api/test_tlm_api.py | 61 +++++++++++++++++++ .../test/utilities/test_questdb_client.py | 23 +++++++ openc3/spec/api/tlm_api_spec.rb | 25 ++++++++ openc3/spec/utilities/questdb_client_spec.rb | 19 ++++++ 14 files changed, 264 insertions(+), 34 deletions(-) diff --git a/openc3-cosmos-cmd-tlm-api/app/models/streaming_object.rb b/openc3-cosmos-cmd-tlm-api/app/models/streaming_object.rb index 31305f8baf..5592e6dbb1 100644 --- a/openc3-cosmos-cmd-tlm-api/app/models/streaming_object.rb +++ b/openc3-cosmos-cmd-tlm-api/app/models/streaming_object.rb @@ -79,9 +79,9 @@ def initialize(key, start_time_nsec, end_time_nsec, item_key: nil, source_key: n @offset = "0-0" @offset = OpenC3::Topic.get_last_offset(@topic, db_shard: @db_shard) unless @start_time if @item_key - # Include the item_key so a LATEST item (which expands to concrete packet keys) - # doesn't collide with an explicit subscription to the same packet item - @id = "ITEM__#{key}__#{@item_key}" + # Include the source_key and item_key so a LATEST item (which expands to concrete + # packet keys) doesn't collide with an explicit subscription to the same packet item + @id = "ITEM__#{key}__#{@source_key}__#{@item_key}" else @id = 'PACKET__' + key end diff --git a/openc3-cosmos-cmd-tlm-api/app/models/streaming_object_collection.rb b/openc3-cosmos-cmd-tlm-api/app/models/streaming_object_collection.rb index 92b6ecb6e4..90197ec8ad 100644 --- a/openc3-cosmos-cmd-tlm-api/app/models/streaming_object_collection.rb +++ b/openc3-cosmos-cmd-tlm-api/app/models/streaming_object_collection.rb @@ -58,16 +58,17 @@ def add(object) def remove(object) @mutex.synchronize do found_object = @objects_by_id[object.id] - if found_object - _remove_found(found_object) - elsif object.item_key - # Items can be added as [key, item_key] pairs but removed by key alone, - # so fall back to removing every item object subscribed with the same key. - # Match on source_key so removing an explicit item doesn't also remove a - # LATEST subscription which expanded to the same concrete key (and vice versa) + # Items can be added as [key, item_key] pairs but removed by key alone + # (the item_key defaults to the key) so remove every item object subscribed with the same key. + # Match on source_key so removing an explicit item doesn't also remove a + # LATEST subscription which expanded to the same concrete key (and vice versa) + key_only = (object.item_key and object.item_key.upcase == object.source_key) + if object.item_key and (key_only or not found_object) @objects.select { |obj| obj.item_key and obj.source_key == object.source_key }.each do |obj| _remove_found(obj) end + elsif found_object + _remove_found(found_object) end end end diff --git a/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb b/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb index e85f80e091..c9e1961068 100644 --- a/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb +++ b/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb @@ -575,6 +575,26 @@ def build(items) build([latest_key]).objects.each { |o| collection.remove(o) } expect(collection.objects.map { |o| [o.key, o.item_key] }).to eq([[explicit_key, explicit_key]]) end + + it 'removes every item_key for a key when removed by key alone' do + collection = build([explicit_key, [explicit_key, '0']]) + expect(collection.length).to eq(2) + build([explicit_key]).objects.each { |o| collection.remove(o) } + expect(collection).to be_empty + end + + it 'removing a key with an item_key only removes that item_key' do + collection = build([explicit_key, [explicit_key, '0']]) + build([[explicit_key, '0']]).objects.each { |o| collection.remove(o) } + expect(collection.objects.map(&:item_key)).to eq([explicit_key]) + end + + it 'keeps a LATEST item and an explicit item with the same item_key' do + collection = build([[latest_key, '0'], [explicit_key, '0']]) + expect(collection.length).to eq(3) + build([[latest_key, '0']]).objects.each { |o| collection.remove(o) } + expect(collection.objects.map { |o| [o.key, o.source_key] }).to eq([[explicit_key, explicit_key]]) + end end describe 'add with COSMOS_ALL packets and authorization' do diff --git a/openc3/lib/openc3/api/tlm_api.rb b/openc3/lib/openc3/api/tlm_api.rb index 3371276679..8677e055a6 100644 --- a/openc3/lib/openc3/api/tlm_api.rb +++ b/openc3/lib/openc3/api/tlm_api.rb @@ -701,18 +701,26 @@ def _tlm_latest_history(cvt_items, start_time:, end_time:, cache_timeout:, scope latest = cvt_items.each_index.select { |index| cvt_items[index].length == 6 } regular = cvt_items.each_index.reject { |index| cvt_items[index].length == 6 } - if end_time - # The QuestDB lookup ASOF JOINs every packet to the rows of the first, so - # a range needs a regular item first to drive the rows. Without one keep - # the realtime behavior of using the packet received most recently now. - if regular.all? { |index| cvt_items[index][2].nil? } - cvt_items = cvt_items.map do |target_name, packet_name, item_name, value_type, limits, _| - packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout: cache_timeout, scope: scope) if packet_name == 'LATEST' - [target_name, packet_name, item_name, value_type, limits] - end - return CvtModel.tsdb_lookup(cvt_items, start_time: start_time, end_time: end_time, scope: scope) + # The QuestDB lookup ASOF JOINs every packet to the rows of the first, so + # a range needs a regular item first to drive the rows. Without one keep + # the realtime behavior of using the packet received most recently now. + if end_time and regular.all? { |index| cvt_items[index][2].nil? } + cvt_items = cvt_items.map do |target_name, packet_name, item_name, value_type, limits, _| + packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout: cache_timeout, scope: scope) if packet_name == 'LATEST' + [target_name, packet_name, item_name, value_type, limits] end + return CvtModel.tsdb_lookup(cvt_items, start_time: start_time, end_time: end_time, scope: scope) + end + # A packet without a table fails the query so skip those candidates + packets = latest.flat_map { |index| cvt_items[index][5].map { |candidate| candidate[0..1] } }.uniq + existing = CvtModel.tsdb_packets_with_tables(packets, scope: scope) + cvt_items = cvt_items.each_with_index.map do |cvt_item, index| + next cvt_item unless latest.include?(index) + cvt_item[0..4] + [cvt_item[5].select { |candidate| existing.include?(candidate[0..1]) }] + end + + if end_time query = [] query_index = {} add = lambda { |query_item| query_index[query_item] ||= (query << query_item; query.length - 1) } diff --git a/openc3/lib/openc3/models/cvt_model.rb b/openc3/lib/openc3/models/cvt_model.rb index 486b879dbd..3cab6a01f0 100644 --- a/openc3/lib/openc3/models/cvt_model.rb +++ b/openc3/lib/openc3/models/cvt_model.rb @@ -138,6 +138,11 @@ def self.tsdb_lookup(items, start_time:, end_time: nil, scope: $openc3_scope, fl QuestDBClient.tsdb_lookup(items, start_time: start_time, end_time: end_time, scope: scope, flatten: flatten) end + # Return the [target_name, packet_name] telemetry packets which have a TSDB table + def self.tsdb_packets_with_tables(packets, scope: $openc3_scope) + QuestDBClient.tlm_packets_with_tables(packets, scope: scope) + end + # Return all item values and limit state from the CVT # # @param items [Array] Items to return. Must be formatted as TGT__PKT__ITEM__TYPE diff --git a/openc3/lib/openc3/utilities/questdb_client.rb b/openc3/lib/openc3/utilities/questdb_client.rb index daec16a6b6..be4ccd0a13 100644 --- a/openc3/lib/openc3/utilities/questdb_client.rb +++ b/openc3/lib/openc3/utilities/questdb_client.rb @@ -632,6 +632,23 @@ def self.table_has_data?(table_name, start_time, end_time, db_shard: 0) false end + # Return the telemetry packets which have a table. The TSDB microservice only + # creates tables for its initial packets so a packet added later has no table + # until it's received. + # + # @param packets [Array] [target_name, packet_name] pairs + # @return [Set] The pairs which have a table + def self.tlm_packets_with_tables(packets, scope: "DEFAULT") + existing = Set.new + packets.group_by { |target_name, _| db_shard_for_target(target_name, scope: scope) }.each do |db_shard, shard_packets| + table_names = shard_packets.to_h { |target_name, packet_name| [sanitize_table_name(target_name, packet_name, scope: scope), [target_name, packet_name]] } + placeholders = (1..table_names.length).map { |index| "$#{index}" }.join(", ") + result = query_with_retry("SELECT table_name FROM tables() WHERE table_name IN (#{placeholders})", params: table_names.keys, label: "tables", db_shard: db_shard) + result.each { |row| existing << table_names[row['table_name']] } + end + existing + end + # QuestDB column types that can be aggregated with min/max/avg/stddev. # Used by reduced queries to decide whether a converted (__C) column is # numeric or must fall back to the raw column (e.g. states stored as VARCHAR). diff --git a/openc3/python/openc3/api/tlm_api.py b/openc3/python/openc3/api/tlm_api.py index 134ecd09b3..06674e626d 100644 --- a/openc3/python/openc3/api/tlm_api.py +++ b/openc3/python/openc3/api/tlm_api.py @@ -826,21 +826,35 @@ def _tlm_available_value_type(item_config, item_name, value_type): def _tlm_latest_history(cvt_items, latest_candidates, start_time, end_time, cache_timeout, scope): regular = [index for index in range(len(cvt_items)) if index not in latest_candidates] - if end_time is not None: - # The QuestDB lookup ASOF JOINs every packet to the rows of the first, so - # a range needs a regular item first to drive the rows. Without one keep - # the realtime behavior of using the packet received most recently now. - if all(cvt_items[index].item_name is None for index in regular): - resolved = [] - for cvt_item in cvt_items: - if cvt_item.packet_name == "LATEST": - packet_name = CvtModel.determine_latest_packet_for_item( - cvt_item.target_name, cvt_item.item_name, cache_timeout, scope - ) - cvt_item = cvt_item._replace(packet_name=packet_name) - resolved.append(cvt_item) - return CvtModel.tsdb_lookup(resolved, start_time=start_time, end_time=end_time, scope=scope) + # The QuestDB lookup ASOF JOINs every packet to the rows of the first, so + # a range needs a regular item first to drive the rows. Without one keep + # the realtime behavior of using the packet received most recently now. + if end_time is not None and all(cvt_items[index].item_name is None for index in regular): + resolved = [] + for cvt_item in cvt_items: + if cvt_item.packet_name == "LATEST": + packet_name = CvtModel.determine_latest_packet_for_item( + cvt_item.target_name, cvt_item.item_name, cache_timeout, scope + ) + cvt_item = cvt_item._replace(packet_name=packet_name) + resolved.append(cvt_item) + return CvtModel.tsdb_lookup(resolved, start_time=start_time, end_time=end_time, scope=scope) + + # A packet without a table fails the query so skip those candidates + packets = list( + dict.fromkeys( + (candidate.target_name, candidate.packet_name) + for candidates in latest_candidates.values() + for candidate in candidates + ) + ) + existing = CvtModel.tsdb_packets_with_tables(packets, scope=scope) + latest_candidates = { + index: [candidate for candidate in candidates if (candidate.target_name, candidate.packet_name) in existing] + for index, candidates in latest_candidates.items() + } + if end_time is not None: query = [] query_index = {} diff --git a/openc3/python/openc3/models/cvt_model.py b/openc3/python/openc3/models/cvt_model.py index 46b529b93e..59a52b098e 100644 --- a/openc3/python/openc3/models/cvt_model.py +++ b/openc3/python/openc3/models/cvt_model.py @@ -197,6 +197,11 @@ def tsdb_lookup( """Query historical telemetry data from TSDB""" return QuestDBClient.tsdb_lookup(items, start_time=start_time, end_time=end_time, scope=scope, flatten=flatten) + @classmethod + def tsdb_packets_with_tables(cls, packets: list, scope: str = OPENC3_SCOPE): + """Return the (target_name, packet_name) telemetry packets which have a TSDB table""" + return QuestDBClient.tlm_packets_with_tables(packets, scope=scope) + # Return all item values and limit state from the CVT # # @param items [Array] Items to return. Must be formatted as TGT__PKT__ITEM__TYPE diff --git a/openc3/python/openc3/utilities/questdb_client.py b/openc3/python/openc3/utilities/questdb_client.py index 0652e1e736..fb8ff794b5 100644 --- a/openc3/python/openc3/utilities/questdb_client.py +++ b/openc3/python/openc3/utilities/questdb_client.py @@ -780,6 +780,36 @@ def db_shard_for_target(cls, target_name, scope="DEFAULT"): return Store.db_shard_for_target(target_name, scope=scope) + @classmethod + def tlm_packets_with_tables(cls, packets, scope="DEFAULT"): + """Return the telemetry packets which have a table. The TSDB microservice only + creates tables for its initial packets so a packet added later has no table + until it's received. + + Args: + packets: Iterable of (target_name, packet_name) pairs + scope: Scope name + + Returns: + Set of the (target_name, packet_name) pairs which have a table + """ + by_db_shard = {} + for target_name, packet_name in packets: + table_name, _ = cls.sanitize_table_name(target_name, packet_name, scope=scope) + db_shard = cls.db_shard_for_target(target_name, scope=scope) + by_db_shard.setdefault(db_shard, {})[table_name] = (target_name, packet_name) + existing = set() + for db_shard, table_names in by_db_shard.items(): + placeholders = ", ".join(["%s"] * len(table_names)) + rows = cls.query_with_retry( + f"SELECT table_name FROM tables() WHERE table_name IN ({placeholders})", + params=list(table_names), + label="tables", + db_shard=db_shard, + ) + existing.update(table_names[row["table_name"]] for row in rows) + return existing + @classmethod def tsdb_lookup(cls, items, start_time, end_time=None, scope="DEFAULT", flatten=True): """Query historical telemetry data from QuestDB for a list of items. diff --git a/openc3/python/test/api/test_limits_api.py b/openc3/python/test/api/test_limits_api.py index 238731ca29..e6298f2aea 100644 --- a/openc3/python/test/api/test_limits_api.py +++ b/openc3/python/test/api/test_limits_api.py @@ -30,6 +30,8 @@ class TestLimitsApi(unittest.TestCase): def setUp(self, system): redis = mock_redis(self) setup_system() + # Other tests can leave a fake item map in the class level cache + TargetModel.item_map_cache = {} orig_xread = redis.xread diff --git a/openc3/python/test/api/test_tlm_api.py b/openc3/python/test/api/test_tlm_api.py index 6c4ebf7e32..5b538d616b 100644 --- a/openc3/python/test/api/test_tlm_api.py +++ b/openc3/python/test/api/test_tlm_api.py @@ -46,8 +46,12 @@ def packet_item(target_name, packet_name, item_name, type="TLM", scope="DEFAULT" def item_map(target_name, scope="DEFAULT"): return {"TEMP1": ["HEALTH_STATUS", "PARAMS"]} + def packets_with_tables(packets, scope="DEFAULT"): + return set(packets) + def decorator(func): func = patch.object(TargetModel, "packet_item", new=packet_item)(func) + func = patch.object(CvtModel, "tsdb_packets_with_tables", new=packets_with_tables)(func) return patch.object(TargetModel, "get_item_to_packet_map", new=item_map)(func) return decorator @@ -57,6 +61,8 @@ class TestTlmApi(unittest.TestCase): def setUp(self): redis = mock_redis(self) setup_system() + # Other tests can leave a fake item map in the class level cache + TargetModel.item_map_cache = {} self.process = True orig_xread = redis.xread @@ -951,6 +957,61 @@ def test_get_tlm_values_historical_latest_range_of_only_latest_uses_the_current_ self.assertEqual(tsdb_lookup.call_args[0][0], [self._latest_history_item("PARAMS", "TEMP1")]) self.assertNotIn("flatten", tsdb_lookup.call_args.kwargs) + @latest_candidates() + def test_get_tlm_values_historical_latest_skips_candidates_without_a_table_at_a_point_in_time(self): + item = self._latest_history_item + queries = [] + + def tsdb_lookup(query, **_kw): + queries.append(query) + return [[[10.0, None], [100.0, None]]] + + with ( + patch.object(CvtModel, "tsdb_packets_with_tables", return_value={("INST", "HEALTH_STATUS")}) as tables, + patch.object(CvtModel, "tsdb_lookup", side_effect=tsdb_lookup), + ): + vals = get_tlm_values(["INST__LATEST__TEMP1__CONVERTED"], start_time="2026-09-13T00:00:00Z") + tables.assert_called_once_with([("INST", "HEALTH_STATUS"), ("INST", "PARAMS")], scope="DEFAULT") + self.assertEqual( + queries, [[item("HEALTH_STATUS", "TEMP1"), item("HEALTH_STATUS", "PACKET_TIMESECONDS", "RAW")]] + ) + self.assertEqual(vals, [[10.0, None]]) + + @latest_candidates() + def test_get_tlm_values_historical_latest_skips_candidates_without_a_table_in_a_range(self): + item = self._latest_history_item + with ( + patch.object(CvtModel, "tsdb_packets_with_tables", return_value={("INST", "HEALTH_STATUS")}), + patch.object(CvtModel, "tsdb_lookup", return_value=[[[5.0, None], [10.0, None], [300.0, None]]]) as lookup, + ): + vals = get_tlm_values( + ["INST__LATEST__TEMP1__CONVERTED", "INST__HEALTH_STATUS__TEMP2__CONVERTED"], + start_time="2026-09-13T00:00:00Z", + end_time="2026-09-13T01:00:00Z", + ) + self.assertEqual( + lookup.call_args[0][0], + [ + item("HEALTH_STATUS", "TEMP2"), + item("HEALTH_STATUS", "TEMP1"), + item("HEALTH_STATUS", "PACKET_TIMESECONDS", "RAW"), + ], + ) + self.assertEqual(vals, [[10.0, None], [5.0, None]]) + + @latest_candidates() + def test_get_tlm_values_historical_latest_returns_none_when_no_candidate_has_a_table(self): + with ( + patch.object(CvtModel, "tsdb_packets_with_tables", return_value=set()), + patch.object(CvtModel, "tsdb_lookup", return_value=[[[5.0, None]]]) as lookup, + ): + vals = get_tlm_values( + ["INST__LATEST__TEMP1__CONVERTED", "INST__HEALTH_STATUS__TEMP2__CONVERTED"], + start_time="2026-09-13T00:00:00Z", + ) + lookup.assert_called_once() + self.assertEqual(vals, [[None, None], [5.0, None]]) + @latest_candidates(params_temp1=PLAIN_TEMP1) def test_get_tlm_values_historical_latest_queries_each_candidate_for_its_type_and_limits(self): item = self._latest_history_item diff --git a/openc3/python/test/utilities/test_questdb_client.py b/openc3/python/test/utilities/test_questdb_client.py index 6af6144ecd..4dd764c6a5 100644 --- a/openc3/python/test/utilities/test_questdb_client.py +++ b/openc3/python/test/utilities/test_questdb_client.py @@ -143,6 +143,29 @@ def test_queries_and_reconnects_the_given_db_shard(self): disconnect.assert_called_once_with(db_shard=2) +class TestTlmPacketsWithTables(unittest.TestCase): + def test_returns_the_packets_with_a_table_queried_by_db_shard(self): + tables = {0: ["DEFAULT__TLM__INST__HEALTH_STATUS"], 1: ["DEFAULT__TLM__INST2__ADCS"]} + + def query_with_retry(query, params, label, db_shard): + self.assertIn("FROM tables()", query) + self.assertEqual(query.count("%s"), len(params)) + return [{"table_name": table} for table in tables[db_shard] if table in params] + + packets = [("INST", "HEALTH_STATUS"), ("INST", "PARAMS"), ("INST2", "ADCS")] + with ( + patch.object(QuestDBClient, "db_shard_for_target", side_effect=lambda t, scope: 1 if t == "INST2" else 0), + patch.object(QuestDBClient, "query_with_retry", side_effect=query_with_retry), + ): + result = QuestDBClient.tlm_packets_with_tables(packets) + self.assertEqual(result, {("INST", "HEALTH_STATUS"), ("INST2", "ADCS")}) + + def test_does_not_query_without_packets(self): + with patch.object(QuestDBClient, "query_with_retry") as query_with_retry: + self.assertEqual(QuestDBClient.tlm_packets_with_tables([]), set()) + query_with_retry.assert_not_called() + + class TestBuildAggregationSelects(unittest.TestCase): def test_aggregates_raw_column_for_raw_value_type(self): selects, mapping = QuestDBClient.build_aggregation_selects("TEMP1", "RAW") diff --git a/openc3/spec/api/tlm_api_spec.rb b/openc3/spec/api/tlm_api_spec.rb index 7d16e9d407..5eaf8d96a0 100644 --- a/openc3/spec/api/tlm_api_spec.rb +++ b/openc3/spec/api/tlm_api_spec.rb @@ -843,6 +843,7 @@ def test_tlm_unknown(method) temp1 = TargetModel.packet_item('INST', 'HEALTH_STATUS', 'TEMP1', scope: 'DEFAULT') allow(TargetModel).to receive(:packet_item).and_call_original allow(TargetModel).to receive(:packet_item).with('INST', 'PARAMS', 'TEMP1', scope: 'DEFAULT').and_return(temp1) + allow(CvtModel).to receive(:tsdb_packets_with_tables) { |packets, **| packets.to_set } end def item(packet_name, item_name, value_type = "CONVERTED") @@ -916,6 +917,30 @@ def item(packet_name, item_name, value_type = "CONVERTED") ) end + it "skips candidate packets without a table at a point in time" do + expect(CvtModel).to receive(:tsdb_packets_with_tables).with([["INST", "HEALTH_STATUS"], ["INST", "PARAMS"]], scope: "DEFAULT").and_return(Set[["INST", "HEALTH_STATUS"]]) + queries = [] + allow(CvtModel).to receive(:tsdb_lookup) { |query, **| queries << query; [[[10.0, nil], [100.0, nil]]] } + expect(@api.get_tlm_values(["INST__LATEST__TEMP1__CONVERTED"], start_time: "2026-09-13T00:00:00Z")).to eql([[10.0, nil]]) + expect(queries).to eql([[item("HEALTH_STATUS", "TEMP1"), item("HEALTH_STATUS", "PACKET_TIMESECONDS", "RAW")]]) + end + + it "skips candidate packets without a table in a range" do + allow(CvtModel).to receive(:tsdb_packets_with_tables).and_return(Set[["INST", "HEALTH_STATUS"]]) + expect(CvtModel).to receive(:tsdb_lookup).with( + [item("HEALTH_STATUS", "TEMP2"), item("HEALTH_STATUS", "TEMP1"), item("HEALTH_STATUS", "PACKET_TIMESECONDS", "RAW")], + start_time: "2026-09-13T00:00:00Z", end_time: "2026-09-13T01:00:00Z", scope: "DEFAULT", flatten: false + ).and_return([[[5.0, nil], [10.0, nil], [300.0, nil]]]) + vals = @api.get_tlm_values(items, start_time: "2026-09-13T00:00:00Z", end_time: "2026-09-13T01:00:00Z") + expect(vals).to eql([[10.0, nil], [5.0, nil]]) + end + + it "returns nil for a LATEST item when no candidate has a table" do + allow(CvtModel).to receive(:tsdb_packets_with_tables).and_return(Set.new) + expect(CvtModel).to receive(:tsdb_lookup).once.with([item("HEALTH_STATUS", "TEMP2")], any_args).and_return([[[5.0, nil]]]) + expect(@api.get_tlm_values(items, start_time: "2026-09-13T00:00:00Z")).to eql([[nil, nil], [5.0, nil]]) + end + it "authorizes every candidate packet" do allow(CvtModel).to receive(:tsdb_lookup).and_return({}) expect(@api).to receive(:authorize).with(hash_including(packet_name: "HEALTH_STATUS")) diff --git a/openc3/spec/utilities/questdb_client_spec.rb b/openc3/spec/utilities/questdb_client_spec.rb index 888d356b8b..f70ecc126e 100644 --- a/openc3/spec/utilities/questdb_client_spec.rb +++ b/openc3/spec/utilities/questdb_client_spec.rb @@ -91,6 +91,25 @@ def lookup(results, end_time: nil, flatten: true) end end + describe "tlm_packets_with_tables" do + it "returns the packets with a table queried by db_shard" do + allow(QuestDBClient).to receive(:db_shard_for_target) { |target_name, **| target_name == "INST2" ? 1 : 0 } + allow(QuestDBClient).to receive(:query_with_retry) do |query, params:, db_shard:, **| + expect(query).to include("FROM tables()") + tables = { 0 => ["DEFAULT__TLM__INST__HEALTH_STATUS"], 1 => ["DEFAULT__TLM__INST2__ADCS"] }[db_shard] + expect(params.length).to eql(db_shard == 0 ? 2 : 1) + tables.select { |table| params.include?(table) }.map { |table| { "table_name" => table } } + end + packets = [["INST", "HEALTH_STATUS"], ["INST", "PARAMS"], ["INST2", "ADCS"]] + expect(QuestDBClient.tlm_packets_with_tables(packets)).to eql(Set[["INST", "HEALTH_STATUS"], ["INST2", "ADCS"]]) + end + + it "doesn't query without packets" do + expect(QuestDBClient).not_to receive(:query_with_retry) + expect(QuestDBClient.tlm_packets_with_tables([])).to eql(Set.new) + end + end + describe "numeric_column_type?" do it "returns true for aggregatable numeric types (case-insensitive)" do ['BYTE', 'SHORT', 'INT', 'LONG', 'FLOAT', 'DOUBLE', 'double', 'float'].each do |type| From 8e4a212a9358144a0b517610054e78b93f402733 Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Sun, 18 Oct 2026 21:19:51 -0600 Subject: [PATCH 11/14] fix(test): keep internal helper out of the python api docs check The api docs spec reads every def as a public API, so the nested add helper in tlm_api.py failed the doc and Ruby/Python parity checks. Match the skip filters on file basename so a checkout path containing "stream" no longer drops every Python script file. Co-Authored-By: Claude Opus 5.5 --- openc3/python/openc3/api/tlm_api.py | 10 ++++++---- openc3/spec/install/api_docs_spec.rb | 24 ++++++++++++------------ 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/openc3/python/openc3/api/tlm_api.py b/openc3/python/openc3/api/tlm_api.py index 06674e626d..c816a8a53f 100644 --- a/openc3/python/openc3/api/tlm_api.py +++ b/openc3/python/openc3/api/tlm_api.py @@ -858,7 +858,7 @@ def _tlm_latest_history(cvt_items, latest_candidates, start_time, end_time, cach query = [] query_index = {} - def add(query_item): + def _add_query_item(query_item): if query_item not in query_index: query_index[query_item] = len(query) query.append(query_item) @@ -866,12 +866,14 @@ def add(query_item): positions = [None] * len(cvt_items) for index in regular: - positions[index] = add(cvt_items[index]) + positions[index] = _add_query_item(cvt_items[index]) for index, candidates in latest_candidates.items(): positions[index] = [ ( - add(candidate), - add(TlmItem(candidate.target_name, candidate.packet_name, "PACKET_TIMESECONDS", "RAW", None)), + _add_query_item(candidate), + _add_query_item( + TlmItem(candidate.target_name, candidate.packet_name, "PACKET_TIMESECONDS", "RAW", None) + ), ) for candidate in candidates ] diff --git a/openc3/spec/install/api_docs_spec.rb b/openc3/spec/install/api_docs_spec.rb index 670cea8167..4b9dede2b6 100644 --- a/openc3/spec/install/api_docs_spec.rb +++ b/openc3/spec/install/api_docs_spec.rb @@ -59,14 +59,14 @@ def parse_file(filename, methods) def ruby_api ruby_api = {} Dir[File.join(SPEC_DIR,'../lib/openc3/script/*.rb')].each do |filename| - next if filename.include?('extract') - next if filename.include?('web_socket_api') - next if filename.include?('suite_results') - next if filename.include?('suite_runner') + next if File.basename(filename).include?('extract') + next if File.basename(filename).include?('web_socket_api') + next if File.basename(filename).include?('suite_results') + next if File.basename(filename).include?('suite_runner') parse_file(filename, ruby_api) end Dir[File.join(SPEC_DIR,'../lib/openc3/api/*.rb')].each do |filename| - next if filename.include?('metrics_api') # TODO: document and implement Python equivalent + next if File.basename(filename).include?('metrics_api') # TODO: document and implement Python equivalent parse_file(filename, ruby_api) end return ruby_api @@ -75,13 +75,13 @@ def ruby_api def python_api python_api = {} Dir[File.join(SPEC_DIR,'../python/openc3/script/*.py')].each do |filename| - next if filename.include?('authorization') - next if filename.include?('decorators') - next if filename.include?('server_proxy') - next if filename.include?('stream') - next if filename.include?('web_socket_api') - next if filename.include?('suite_results') - next if filename.include?('suite_runner') + next if File.basename(filename).include?('authorization') + next if File.basename(filename).include?('decorators') + next if File.basename(filename).include?('server_proxy') + next if File.basename(filename).include?('stream') + next if File.basename(filename).include?('web_socket_api') + next if File.basename(filename).include?('suite_results') + next if File.basename(filename).include?('suite_runner') parse_file(filename, python_api) end Dir[File.join(SPEC_DIR,'../python/openc3/api/*.py')].each do |filename| From 36cf9495998ca8cf6740f927dc72cfede71e6455 Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Sun, 18 Oct 2026 21:19:51 -0600 Subject: [PATCH 12/14] fix(telemetry): query the current LATEST packet for its own type and limits A range of only LATEST items resolved the packet received most recently but queried it with the requested value type and limits, so a packet without those columns failed the whole QuestDB query. Use the matching candidate, which carries the type and limits that packet has. Co-Authored-By: Claude Opus 5.5 --- openc3/lib/openc3/api/tlm_api.rb | 8 +++++--- openc3/python/openc3/api/tlm_api.py | 8 ++++++-- openc3/python/test/api/test_tlm_api.py | 13 +++++++++++++ openc3/spec/api/tlm_api_spec.rb | 10 ++++++++++ 4 files changed, 34 insertions(+), 5 deletions(-) diff --git a/openc3/lib/openc3/api/tlm_api.rb b/openc3/lib/openc3/api/tlm_api.rb index 8677e055a6..24284785f6 100644 --- a/openc3/lib/openc3/api/tlm_api.rb +++ b/openc3/lib/openc3/api/tlm_api.rb @@ -705,9 +705,11 @@ def _tlm_latest_history(cvt_items, start_time:, end_time:, cache_timeout:, scope # a range needs a regular item first to drive the rows. Without one keep # the realtime behavior of using the packet received most recently now. if end_time and regular.all? { |index| cvt_items[index][2].nil? } - cvt_items = cvt_items.map do |target_name, packet_name, item_name, value_type, limits, _| - packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout: cache_timeout, scope: scope) if packet_name == 'LATEST' - [target_name, packet_name, item_name, value_type, limits] + cvt_items = cvt_items.map do |target_name, packet_name, item_name, value_type, limits, candidates| + next [target_name, packet_name, item_name, value_type, limits] unless packet_name == 'LATEST' + packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout: cache_timeout, scope: scope) + # Use the candidate so the query asks for the value type and limits that packet has + candidates.find { |candidate| candidate[1] == packet_name } || [target_name, packet_name, item_name, value_type, limits] end return CvtModel.tsdb_lookup(cvt_items, start_time: start_time, end_time: end_time, scope: scope) end diff --git a/openc3/python/openc3/api/tlm_api.py b/openc3/python/openc3/api/tlm_api.py index c816a8a53f..38e3c7d20b 100644 --- a/openc3/python/openc3/api/tlm_api.py +++ b/openc3/python/openc3/api/tlm_api.py @@ -831,12 +831,16 @@ def _tlm_latest_history(cvt_items, latest_candidates, start_time, end_time, cach # the realtime behavior of using the packet received most recently now. if end_time is not None and all(cvt_items[index].item_name is None for index in regular): resolved = [] - for cvt_item in cvt_items: + for index, cvt_item in enumerate(cvt_items): if cvt_item.packet_name == "LATEST": packet_name = CvtModel.determine_latest_packet_for_item( cvt_item.target_name, cvt_item.item_name, cache_timeout, scope ) - cvt_item = cvt_item._replace(packet_name=packet_name) + # Use the candidate so the query asks for the value type and limits that packet has + cvt_item = next( + (candidate for candidate in latest_candidates[index] if candidate.packet_name == packet_name), + cvt_item._replace(packet_name=packet_name), + ) resolved.append(cvt_item) return CvtModel.tsdb_lookup(resolved, start_time=start_time, end_time=end_time, scope=scope) diff --git a/openc3/python/test/api/test_tlm_api.py b/openc3/python/test/api/test_tlm_api.py index 5b538d616b..6d2f5cba87 100644 --- a/openc3/python/test/api/test_tlm_api.py +++ b/openc3/python/test/api/test_tlm_api.py @@ -1034,6 +1034,19 @@ def tsdb_lookup(query, **_kw): ], ) + @latest_candidates(params_temp1=PLAIN_TEMP1) + def test_get_tlm_values_historical_latest_range_of_only_latest_uses_the_current_packet_type_and_limits(self): + with ( + patch.object(CvtModel, "determine_latest_packet_for_item", return_value="PARAMS"), + patch.object(CvtModel, "tsdb_lookup", return_value=[]) as tsdb_lookup, + ): + get_tlm_values( + ["INST__LATEST__TEMP1__FORMATTED__LIMITS"], + start_time="2026-09-13T00:00:00Z", + end_time="2026-09-13T01:00:00Z", + ) + self.assertEqual(tsdb_lookup.call_args[0][0], [TlmItem("INST", "PARAMS", "TEMP1", "RAW", None)]) + @latest_candidates(params_temp1=PLAIN_TEMP1) def test_get_tlm_available_returns_the_best_type_and_limits_of_any_latest_packet(self): with patch.object(CvtModel, "determine_latest_packet_for_item", return_value="PARAMS"): diff --git a/openc3/spec/api/tlm_api_spec.rb b/openc3/spec/api/tlm_api_spec.rb index 5eaf8d96a0..debc8f114e 100644 --- a/openc3/spec/api/tlm_api_spec.rb +++ b/openc3/spec/api/tlm_api_spec.rb @@ -917,6 +917,16 @@ def item(packet_name, item_name, value_type = "CONVERTED") ) end + it "queries the current packet for the value type and limits it has in a range of only LATEST items" do + plain = { 'name' => 'TEMP1', 'data_type' => 'UINT', 'limits' => {} } + allow(TargetModel).to receive(:packet_item).with('INST', 'PARAMS', 'TEMP1', scope: 'DEFAULT').and_return(plain) + allow(CvtModel).to receive(:determine_latest_packet_for_item).and_return("PARAMS") + expect(CvtModel).to receive(:tsdb_lookup).with( + [["INST", "PARAMS", "TEMP1", "RAW", nil]], start_time: "2026-09-13T00:00:00Z", end_time: "2026-09-13T01:00:00Z", scope: "DEFAULT" + ).and_return([]) + @api.get_tlm_values(["INST__LATEST__TEMP1__FORMATTED__LIMITS"], start_time: "2026-09-13T00:00:00Z", end_time: "2026-09-13T01:00:00Z") + end + it "skips candidate packets without a table at a point in time" do expect(CvtModel).to receive(:tsdb_packets_with_tables).with([["INST", "HEALTH_STATUS"], ["INST", "PARAMS"]], scope: "DEFAULT").and_return(Set[["INST", "HEALTH_STATUS"]]) queries = [] From 96c78db900f501927e0917e38e790944194829d8 Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Wed, 21 Oct 2026 11:59:02 -0600 Subject: [PATCH 13/14] fix(streaming): match item_key in fallback removal sweep Removing an explicit [key, item_key] pair that has no exact object now only sweeps objects with the same source_key and item_key, so an unknown pair can no longer drop other subscriptions on that key. Co-Authored-By: Claude --- .../app/models/streaming_object_collection.rb | 6 +++++- .../spec/models/streaming_api_spec.rb | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/openc3-cosmos-cmd-tlm-api/app/models/streaming_object_collection.rb b/openc3-cosmos-cmd-tlm-api/app/models/streaming_object_collection.rb index 90197ec8ad..2398241bd2 100644 --- a/openc3-cosmos-cmd-tlm-api/app/models/streaming_object_collection.rb +++ b/openc3-cosmos-cmd-tlm-api/app/models/streaming_object_collection.rb @@ -64,7 +64,11 @@ def remove(object) # LATEST subscription which expanded to the same concrete key (and vice versa) key_only = (object.item_key and object.item_key.upcase == object.source_key) if object.item_key and (key_only or not found_object) - @objects.select { |obj| obj.item_key and obj.source_key == object.source_key }.each do |obj| + # An explicit item_key that wasn't found exactly must also match on item_key + # so an unknown [key, item_key] pair can't remove other subscriptions + @objects.select do |obj| + obj.item_key and obj.source_key == object.source_key and (key_only or obj.item_key == object.item_key) + end.each do |obj| _remove_found(obj) end elsif found_object diff --git a/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb b/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb index a23f444c68..14231fb884 100644 --- a/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb +++ b/openc3-cosmos-cmd-tlm-api/spec/models/streaming_api_spec.rb @@ -642,6 +642,25 @@ def build(items) expect(collection.objects.map(&:item_key)).to eq([explicit_key]) end + it 'removing an unknown item_key does not remove other item_keys' do + collection = build([explicit_key, [explicit_key, '0']]) + build([[explicit_key, '1']]).objects.each { |o| collection.remove(o) } + expect(collection.objects.map(&:item_key)).to contain_exactly(explicit_key, '0') + end + + it 'falls back to matching source_key and item_key when the exact object is not found' do + allow(OpenC3::TargetModel).to receive(:get_item_to_packet_map).with('INST', scope: 'DEFAULT').and_return({ + 'TEMP1' => ['PARAMS'], + }) + collection = build([[latest_key, '0'], [latest_key, '1']]) + allow(OpenC3::TargetModel).to receive(:get_item_to_packet_map).with('INST', scope: 'DEFAULT').and_return({ + 'TEMP1' => ['HEALTH_STATUS'], + }) + # The HEALTH_STATUS object was never added so the removal falls back to the sweep + build([[latest_key, '0']]).objects.each { |o| collection.remove(o) } + expect(collection.objects.map(&:item_key)).to eq(['1']) + end + it 'keeps a LATEST item and an explicit item with the same item_key' do collection = build([[latest_key, '0'], [explicit_key, '0']]) expect(collection.length).to eq(3) From 2d198d83b99a800f71de257019a4f3db57c9faee Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Wed, 21 Oct 2026 11:59:02 -0600 Subject: [PATCH 14/14] fix(api): type LATEST history positions without None placeholders Build positions in _tlm_latest_history as a dict keyed by item index so ty no longer flags a possibly-None entry as not iterable. Co-Authored-By: Claude --- openc3/python/openc3/api/tlm_api.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/openc3/python/openc3/api/tlm_api.py b/openc3/python/openc3/api/tlm_api.py index 38e3c7d20b..8663c557fb 100644 --- a/openc3/python/openc3/api/tlm_api.py +++ b/openc3/python/openc3/api/tlm_api.py @@ -868,7 +868,8 @@ def _add_query_item(query_item): query.append(query_item) return query_index[query_item] - positions = [None] * len(cvt_items) + # Regular items go in the query first so the first packet drives the rows + positions: dict[int, int | list[tuple[int, int]]] = {} for index in regular: positions[index] = _add_query_item(cvt_items[index]) for index, candidates in latest_candidates.items(): @@ -887,7 +888,8 @@ def _add_query_item(query_item): collapsed = [] for row in rows: values = [] - for position in positions: + for index in range(len(cvt_items)): + position = positions[index] if isinstance(position, int): values.append(row[position]) else: