Merge pull request #1577 from OpenC3/deterministic_recalculate_bit_of…

…fsets

Deterministic recalculate bit offsets

openc3/ext/openc3/ext/structure/structure.c

@@ -15,7 +15,7 @@
 
 /*
 # Modified by OpenC3, Inc.
-# All changes Copyright 2022, OpenC3, Inc.
+# All changes Copyright 2024, OpenC3, Inc.
 # All Rights Reserved
 #
 # This file may also be used under the terms of a commercial license
@@ -69,7 +69,9 @@ static ID id_method_class2 = 0;
 
 static ID id_ivar_buffer = 0;
 static ID id_ivar_bit_offset = 0;
+static ID id_ivar_original_bit_offset = 0;
 static ID id_ivar_bit_size = 0;
+static ID id_ivar_variable_bit_size = 0;
 static ID id_ivar_array_size = 0;
 static ID id_ivar_endianness = 0;
 static ID id_ivar_data_type = 0;
@@ -1360,92 +1362,86 @@ static VALUE read_item(int argc, VALUE *argv, VALUE self)
  */
 static VALUE structure_item_spaceship(VALUE self, VALUE other_item)
 {
-  int bit_offset = 0;
-  int other_bit_offset = 0;
-  int bit_size = 0;
-  int other_bit_size = 0;
+  int original_bit_offset = 0;
+  int other_original_bit_offset = 0;
   int create_index = 0;
   int other_create_index = 0;
-  int have_create_index = 0;
-  volatile VALUE v_create_index = Qnil;
-  volatile VALUE v_other_create_index = Qnil;
+  volatile VALUE data_type = Qnil;
+  volatile VALUE other_data_type = Qnil;
+  volatile VALUE variable_bit_size = Qnil;
+  volatile VALUE other_variable_bit_size = Qnil;
 
   if (!RTEST(rb_funcall(other_item, id_method_kind_of, 1, cStructureItem)))
   {
     return Qnil;
   }
 
-  bit_offset = FIX2INT(rb_ivar_get(self, id_ivar_bit_offset));
-  other_bit_offset = FIX2INT(rb_ivar_get(other_item, id_ivar_bit_offset));
+  original_bit_offset = FIX2INT(rb_ivar_get(self, id_ivar_original_bit_offset));
+  other_original_bit_offset = FIX2INT(rb_ivar_get(other_item, id_ivar_original_bit_offset));
 
-  v_create_index = rb_ivar_get(self, id_ivar_create_index);
-  v_other_create_index = rb_ivar_get(other_item, id_ivar_create_index);
-  if (RTEST(v_create_index) && RTEST(v_other_create_index))
-  {
-    create_index = FIX2INT(v_create_index);
-    other_create_index = FIX2INT(v_other_create_index);
-    have_create_index = 1;
-  }
+  create_index = FIX2INT(rb_ivar_get(self, id_ivar_create_index));
+  other_create_index = FIX2INT(rb_ivar_get(other_item, id_ivar_create_index));
+
+  data_type = rb_ivar_get(self, id_ivar_data_type);
+  other_data_type = rb_ivar_get(other_item, id_ivar_data_type);
 
-  /* Handle same bit offset case */
-  if ((bit_offset == 0) && (other_bit_offset == 0))
+  variable_bit_size = rb_ivar_get(self, id_ivar_variable_bit_size);
+  other_variable_bit_size = rb_ivar_get(other_item, id_ivar_variable_bit_size);
+
+  /* Derived items should be first in the list with multiple derived sorted
+   * by create_index */
+  if (data_type == symbol_DERIVED)
   {
-    /* Both bit_offsets are 0 so sort by bit_size
-     * This allows derived items with bit_size of 0 to be listed first
-     * Compare based on bit size */
-    bit_size = FIX2INT(rb_ivar_get(self, id_ivar_bit_size));
-    other_bit_size = FIX2INT(rb_ivar_get(other_item, id_ivar_bit_size));
-    if (bit_size == other_bit_size)
+    if (other_data_type != symbol_DERIVED)
     {
-      if (have_create_index)
+      return INT2FIX(-1);
+    }
+    else
+    {
+      if (create_index <= other_create_index)
       {
-        if (create_index <= other_create_index)
-        {
-          return INT2FIX(-1);
-        }
-        else
-        {
-          return INT2FIX(1);
-        }
+        return INT2FIX(-1);
       }
       else
       {
-        return INT2FIX(0);
+        return INT2FIX(1);
       }
     }
-    if (bit_size < other_bit_size)
-    {
-      return INT2FIX(-1);
-    }
-    else
-    {
-      return INT2FIX(1);
-    }
+  }
+  else if (other_data_type == symbol_DERIVED)
+  {
+    return INT2FIX(1);
   }
 
-  /* Handle different bit offsets */
-  if (((bit_offset >= 0) && (other_bit_offset >= 0)) || ((bit_offset < 0) && (other_bit_offset < 0)))
+  /* Handle non-derived items */
+  if (((original_bit_offset >= 0) && (other_original_bit_offset >= 0)) || ((original_bit_offset < 0) && (other_original_bit_offset < 0)))
   {
     /* Both Have Same Sign */
-    if (bit_offset == other_bit_offset)
+    if (original_bit_offset == other_original_bit_offset)
     {
-      if (have_create_index)
+      if (RTEST(variable_bit_size))
       {
-        if (create_index <= other_create_index)
+        if (!RTEST(other_variable_bit_size))
         {
           return INT2FIX(-1);
         }
-        else
-        {
-          return INT2FIX(1);
-        }
+        /* If both variable_bit_size use create index */
+      }
+      else if (RTEST(other_variable_bit_size))
+      {
+        return INT2FIX(1);
+      }
+
+      if (create_index <= other_create_index)
+      {
+        return INT2FIX(-1);
       }
       else
       {
-        return INT2FIX(0);
+        return INT2FIX(1);
       }
     }
-    else if (bit_offset < other_bit_offset)
+    else if (original_bit_offset <= other_original_bit_offset)
     {
       return INT2FIX(-1);
     }
@@ -1457,25 +1453,7 @@ static VALUE structure_item_spaceship(VALUE self, VALUE other_item)
   else
   {
     /* Different Signs */
-    if (bit_offset == other_bit_offset)
-    {
-      if (have_create_index)
-      {
-        if (create_index <= other_create_index)
-        {
-          return INT2FIX(-1);
-        }
-        else
-        {
-          return INT2FIX(1);
-        }
-      }
-      else
-      {
-        return INT2FIX(0);
-      }
-    }
-    else if (bit_offset < other_bit_offset)
+    if (original_bit_offset < other_original_bit_offset)
     {
       return INT2FIX(1);
     }
@@ -1649,7 +1627,9 @@ void Init_structure(void)
 
   id_ivar_buffer = rb_intern("@buffer");
   id_ivar_bit_offset = rb_intern("@bit_offset");
+  id_ivar_original_bit_offset = rb_intern("@original_bit_offset");
   id_ivar_bit_size = rb_intern("@bit_size");
+  id_ivar_variable_bit_size = rb_intern("@variable_bit_size");
   id_ivar_array_size = rb_intern("@array_size");
   id_ivar_endianness = rb_intern("@endianness");
   id_ivar_data_type = rb_intern("@data_type");

openc3/lib/openc3/packets/packet.rb

@@ -329,7 +329,7 @@ def buffer=(buffer)
       synchronize() do
         begin
           internal_buffer_equals(buffer)
-        rescue RuntimeError
+        rescue RuntimeError => e
           Logger.instance.error "#{@target_name} #{@packet_name} received with actual packet length of #{buffer.length} but defined length of #{@defined_length}"
         end
         @read_conversion_cache.clear if @read_conversion_cache

openc3/lib/openc3/packets/structure.rb

@@ -617,9 +617,17 @@ def recalculate_bit_offsets
         # Anything with a negative bit offset should be left alone
         if item.original_bit_offset >= 0
           item.bit_offset = item.original_bit_offset + adjustment
-          if item.data_type != :DERIVED and (item.variable_bit_size or item.original_bit_size <= 0 or (item.original_array_size and item.original_array_size <= 0))
+
+          # May need to update adjustment with variable length items
+          # Note legacy variable length does not push anything
+          if item.data_type != :DERIVED and item.variable_bit_size # Not DERIVED and New Variable Length
+            # Calculate the actual current size of this variable length item
             new_bit_size = calculate_total_bit_size(item)
+
             if item.original_bit_size != new_bit_size
+              # Bit size has changed from original - so we need to adjust everything after this item
+              # This includes items that may have the same bit_offset as the variable length item because it
+              # started out at zero bit_size
               adjustment += (new_bit_size - item.original_bit_size)
             end
           end

openc3/lib/openc3/packets/structure_item.rb

@@ -95,6 +95,9 @@ class StructureItem
     # @return [Hash] Variable bit size information
     attr_reader :variable_bit_size
 
+    # @return [Integer] Incrementing value that shows relative order items are created
+    attr_reader :create_index
+
     # Create a StructureItem by setting all the attributes. It
     # calls all the setter routines to do the attribute verification and then
     # verifies the overall integrity.
@@ -238,73 +241,56 @@ def variable_bit_size=(variable_bit_size)
       verify_overall() if @structure_item_constructed
     end
 
-    def create_index
-      @create_index.to_i
-    end
-
     if RUBY_ENGINE != 'ruby' or ENV['OPENC3_NO_EXT']
-      # Comparison Operator based on bit_offset. This means that StructureItems
-      # with different names or bit sizes are equal if they have the same bit
-      # offset.
+      # Comparison Operator primarily based on bit_offset
       def <=>(other)
         return nil unless other.kind_of?(StructureItem)
 
-        other_bit_offset = other.bit_offset
-        other_bit_size = other.bit_size
-
-        # Handle same bit offset case
-        if (@bit_offset == 0) && (other_bit_offset == 0)
-          # Both bit_offsets are 0 so sort by bit_size
-          # This allows derived items with bit_size of 0 to be listed first
-          # Compare based on bit size then create index
-          if @bit_size == other_bit_size
-            if @create_index
-              if @create_index <= other.create_index
-                return -1
-              else
-                return 1
-              end
-            else
-              return 0
-            end
-          elsif @bit_size < other_bit_size
+        other_original_bit_offset = other.original_bit_offset
+
+        # Derived items should be first in the list with multiple derived sorted
+        # by create_index
+        if @data_type == :DERIVED
+          if other.data_type != :DERIVED
             return -1
           else
-            return 1
+            if @create_index <= other.create_index
+              return -1
+            else
+              return 1
+            end
           end
+        elsif other.data_type == :DERIVED
+          return 1
         end
 
-        # Handle different bit offsets
-        if ((@bit_offset >= 0) && (other_bit_offset >= 0)) || ((@bit_offset < 0) && (other_bit_offset < 0))
+        # Handle non-derived items
+        if ((@original_bit_offset >= 0) && (other_original_bit_offset >= 0)) || ((@original_bit_offset < 0) && (other_original_bit_offset < 0))
           # Both Have Same Sign
-          if @bit_offset == other_bit_offset
-            if @create_index
-              if @create_index <= other.create_index
+          if @original_bit_offset == other_original_bit_offset
+            # New Variable Bit Size items are before regular items
+            if @variable_bit_size
+              if not other.variable_bit_size
                 return -1
-              else
-                return 1
               end
+              # If both variable_bit_size use create index
+            elsif other.variable_bit_size
+              return 1
+            end
+
+            if @create_index <= other.create_index
+              return -1
             else
-              return 0
+              return 1
             end
-          elsif @bit_offset <= other_bit_offset
+          elsif @original_bit_offset <= other_original_bit_offset
             return -1
           else
             return 1
           end
         else
           # Different Signs
-          if @bit_offset == other_bit_offset
-            if @create_index
-              if @create_index < other.create_index
-                return -1
-              else
-                return 1
-              end
-            else
-              return 0
-            end
-          elsif @bit_offset < other_bit_offset
+          if @original_bit_offset < other_original_bit_offset
             return 1
           else
             return -1

openc3/spec/accessors/binary_accessor_spec.rb

@@ -225,6 +225,23 @@ module OpenC3
         expect(item2_length.bit_offset).to eq 152
         expect(item2.bit_offset).to eq 168
       end
+
+      it "can define multiple variable sized items with overlaps 1" do
+        item1_length = @packet.define_item("item1_length", 0, 32, :INT)
+        item1 = @packet.define_item("item1", 32, 0, :STRING)
+        item1.variable_bit_size = {'length_item_name' => 'item1_length', 'length_value_bit_offset' => 0, 'length_bits_per_count' => 8}
+        item2 = @packet.define_item("item2", 0, 0, :BLOCK)
+        expect(item1_length.bit_offset).to eq(0)
+        expect(item1.bit_offset).to eq(32)
+        expect(item2.bit_offset).to eq(0)
+        @packet.buffer = "\x00\x00\x00\x06\x48\x45\x4C\x4C\x4F\x00"
+        expect(item1_length.bit_offset).to eq(0)
+        expect(item1.bit_offset).to eq(32)
+        expect(item2.bit_offset).to eq(0)
+        expect(@packet.read("item1_length")).to eql 6
+        expect(@packet.read("item1")).to eql "HELLO"
+        expect(@packet.read("item2")).to eql "\x00\x00\x00\x06\x48\x45\x4C\x4C\x4F\x00"
+      end
     end
 
     describe "read only" do

openc3/spec/packets/packet_config_spec.rb

@@ -1093,7 +1093,7 @@ module OpenC3
           tf.puts "  ITEM item2 0 2 UINT"
           tf.close
           @pc.process_file(tf.path, "TGT1")
-          expect(@pc.warnings[0]).to eql "Bit definition overlap at bit offset 0 for packet TGT1 PKT1 items ITEM1 and ITEM2"
+          expect(@pc.warnings[0]).to eql "Bit definition overlap at bit offset 0 for packet TGT1 PKT1 items ITEM2 and ITEM1"
           tf.unlink
         end
       end