New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xtce explicit byte order list processing isn't correct #411

Closed
severn opened this Issue Mar 20, 2017 · 6 comments

Comments

Projects
None yet
2 participants
@severn

severn commented Mar 20, 2017

The code in packet_config.rb (line 638) which determines if the byte order is little endian seems to be definitely wrong.

In XTCE, big endian is the default ... which you have also.

But then in XTCE an explicit byte order is given by an explicit ByteOrderList -- and then big or little endian by descending or ascending list of integers -- such as 3,2,1,0 or 0,1,2,3. The cheapest check I've come up with is a loop to determine if the byte order list is ascending or descending -- and then a check if the item ends or starts at zero. Other orderings are possible but they would not be big or little endian (and you don't support them).

As best as I can tell in the current code -- big endian is the default, then if the bye order list element is found, it is looped through and if 0 is found -- the ordering is set to little endian...

@ryanatball

This comment has been minimized.

Show comment
Hide comment
@ryanatball

ryanatball Mar 22, 2017

Member

I believe the current logic is correct. Is it not working for you?

The current logic is:

  1. If it finds "ByteOrderList"
  2. Find the First "Byte"
  3. If "byteSignificance" equals 0
  4. Then set to LITTLE_ENDIAN (otherwise remains at BIG_ENDIAN)

The key detail is that it only looks at the first Byte keyword. Little endian will always have 0 first. Single byte types don't matter if they are big or little endian so its ok if they get marked little endian.

Member

ryanatball commented Mar 22, 2017

I believe the current logic is correct. Is it not working for you?

The current logic is:

  1. If it finds "ByteOrderList"
  2. Find the First "Byte"
  3. If "byteSignificance" equals 0
  4. Then set to LITTLE_ENDIAN (otherwise remains at BIG_ENDIAN)

The key detail is that it only looks at the first Byte keyword. Little endian will always have 0 first. Single byte types don't matter if they are big or little endian so its ok if they get marked little endian.

@severn

This comment has been minimized.

Show comment
Hide comment
@severn

severn Mar 23, 2017

I have this (below) and its getting it as little endian. I added some debugging to the code for it but not enough to fully illuminate. I will work on more details there -- possibly tomorrow late. But the print statements I added seem to suggest it loops through the entire list, but I may have done it incorrectly.

xtce:ByteOrderList
<xtce:Byte byteSignificance="1"/>
<xtce:Byte byteSignificance="0"/>
</xtce:ByteOrderList>

severn commented Mar 23, 2017

I have this (below) and its getting it as little endian. I added some debugging to the code for it but not enough to fully illuminate. I will work on more details there -- possibly tomorrow late. But the print statements I added seem to suggest it loops through the entire list, but I may have done it incorrectly.

xtce:ByteOrderList
<xtce:Byte byteSignificance="1"/>
<xtce:Byte byteSignificance="0"/>
</xtce:ByteOrderList>

@severn

This comment has been minimized.

Show comment
Hide comment
@severn

severn Mar 24, 2017

Here is what I see.

Inputs (example -- this is big endian):

<xtce:IntegerParameterType sizeInBits="32" signed="false" validRangeAppliesToCalibrated="true" name="AC046XBSTATType">
                <xtce:UnitSet/>
                <xtce:IntegerDataEncoding encoding="unsigned" sizeInBits="32" bitOrder="mostSignificantBitFirst">
                    <xtce:ByteOrderList>
                        <xtce:Byte byteSignificance="3"/>
                        <xtce:Byte byteSignificance="2"/>
                        <xtce:Byte byteSignificance="1"/>
                        <xtce:Byte byteSignificance="0"/>
                    </xtce:ByteOrderList>
                </xtce:IntegerDataEncoding>
            </xtce:IntegerParameterType>

Modified Source, PacketConfig.rb, from line 638

when 'ByteOrderList'  
		print @current_type.name, " depth ==> " + depth.to_s + "\n"
        xtce_recurse_element(element, depth + 1) do |element, depth|
          if element.name == 'Byte'
            print @current_type.name, " byteSignificance ==> " + element['byteSignificance'].to_i.to_s + "\n"
            if element['byteSignificance'] and element['byteSignificance'].to_i == 0
              print @current_type.name, " byteSignificance ==> 0 found, it must be little endian ... ?\n"
              @current_type.endianness = :LITTLE_ENDIAN
            end
            false
          else
            true
          end
        end
        return false # Already recursed

Debug output:

AC046XBSTATType depth ==> 5
AC046XBSTATType byteSignificance ==> 3
AC046XBSTATType byteSignificance ==> 2
AC046XBSTATType byteSignificance ==> 1
AC046XBSTATType byteSignificance ==> 0
AC046XBSTATType byteSignificance ==> 0 found, it must be little endian ... ?

BTW I tried Logger and IO.write but failed to find a good (easy) way to capture print to a file... can I tell cosmos to log to a file?

severn commented Mar 24, 2017

Here is what I see.

Inputs (example -- this is big endian):

<xtce:IntegerParameterType sizeInBits="32" signed="false" validRangeAppliesToCalibrated="true" name="AC046XBSTATType">
                <xtce:UnitSet/>
                <xtce:IntegerDataEncoding encoding="unsigned" sizeInBits="32" bitOrder="mostSignificantBitFirst">
                    <xtce:ByteOrderList>
                        <xtce:Byte byteSignificance="3"/>
                        <xtce:Byte byteSignificance="2"/>
                        <xtce:Byte byteSignificance="1"/>
                        <xtce:Byte byteSignificance="0"/>
                    </xtce:ByteOrderList>
                </xtce:IntegerDataEncoding>
            </xtce:IntegerParameterType>

Modified Source, PacketConfig.rb, from line 638

when 'ByteOrderList'  
		print @current_type.name, " depth ==> " + depth.to_s + "\n"
        xtce_recurse_element(element, depth + 1) do |element, depth|
          if element.name == 'Byte'
            print @current_type.name, " byteSignificance ==> " + element['byteSignificance'].to_i.to_s + "\n"
            if element['byteSignificance'] and element['byteSignificance'].to_i == 0
              print @current_type.name, " byteSignificance ==> 0 found, it must be little endian ... ?\n"
              @current_type.endianness = :LITTLE_ENDIAN
            end
            false
          else
            true
          end
        end
        return false # Already recursed

Debug output:

AC046XBSTATType depth ==> 5
AC046XBSTATType byteSignificance ==> 3
AC046XBSTATType byteSignificance ==> 2
AC046XBSTATType byteSignificance ==> 1
AC046XBSTATType byteSignificance ==> 0
AC046XBSTATType byteSignificance ==> 0 found, it must be little endian ... ?

BTW I tried Logger and IO.write but failed to find a good (easy) way to capture print to a file... can I tell cosmos to log to a file?

@severn

This comment has been minimized.

Show comment
Hide comment
@severn

severn Mar 27, 2017

here's some hack around code. I believe the real check should be:
1 - if there is no byte order list, it is big endian. (the list must be 2 or greater size if explicit)
2 - if it's descending to zero, then it's big endian
3 - if it's ascending from zero, then it's little endian
4 - otherwise it's a byte order not supported... it should be noted we do have a small number of these, case by case basis. (so raise exception? or just print out log error/warning and keep going...?)

But I didn't do that. I just collect in a list the significances and check the ends for zero... I left the original code in place as a reference pt and all my debugging prints too. Since I don't know ruby well, I suspect this can all be reduced to a single line of code, etc...

 when 'ByteOrderList'  
		print @current_type.name, " depth ==> " + depth.to_s + "\n"
		significanti = Array.new
        xtce_recurse_element(element, depth + 1) do |element, depth|
          if element.name == 'Byte'
            print @current_type.name, " byteSignificance ==> " + element['byteSignificance'].to_i.to_s + "\n"
            significanti.push(element['byteSignificance'].to_i)
            if element['byteSignificance'] and element['byteSignificance'].to_i == 0
              print @current_type.name, " byteSignificance ==> 0 found, it must be little endian ... ?\n"
              @current_type.endianness = :LITTLE_ENDIAN
            end
            false
          else
            true
          end
        end
        print "Significances found ==> ", significanti.size, "\n"
        if significanti.size > 0
        print "want big endian, first byte==> ", significanti.fetch(0), " last byte ==> ", significanti.fetch(significanti.size-1), "\n"
           if significanti.fetch(0) == 0
               print "here\n"
               @current_type.endianness = :LITTLE_ENDIAN
           elsif significanti.fetch(significanti.size-1) == 0
             print "want big endian, last byte==> ", significanti.values_at(significanti.size-1), "\n"
               @current_type.endianness = :BIG_ENDIAN
           else 
               print "Warning: Byte order not supported ==>", "#{significanti}", "\n"
           end
        end
        print "Significances way byte order now ==> ", @current_type.endianness, "\n\n"
        return false # Already recursed

severn commented Mar 27, 2017

here's some hack around code. I believe the real check should be:
1 - if there is no byte order list, it is big endian. (the list must be 2 or greater size if explicit)
2 - if it's descending to zero, then it's big endian
3 - if it's ascending from zero, then it's little endian
4 - otherwise it's a byte order not supported... it should be noted we do have a small number of these, case by case basis. (so raise exception? or just print out log error/warning and keep going...?)

But I didn't do that. I just collect in a list the significances and check the ends for zero... I left the original code in place as a reference pt and all my debugging prints too. Since I don't know ruby well, I suspect this can all be reduced to a single line of code, etc...

 when 'ByteOrderList'  
		print @current_type.name, " depth ==> " + depth.to_s + "\n"
		significanti = Array.new
        xtce_recurse_element(element, depth + 1) do |element, depth|
          if element.name == 'Byte'
            print @current_type.name, " byteSignificance ==> " + element['byteSignificance'].to_i.to_s + "\n"
            significanti.push(element['byteSignificance'].to_i)
            if element['byteSignificance'] and element['byteSignificance'].to_i == 0
              print @current_type.name, " byteSignificance ==> 0 found, it must be little endian ... ?\n"
              @current_type.endianness = :LITTLE_ENDIAN
            end
            false
          else
            true
          end
        end
        print "Significances found ==> ", significanti.size, "\n"
        if significanti.size > 0
        print "want big endian, first byte==> ", significanti.fetch(0), " last byte ==> ", significanti.fetch(significanti.size-1), "\n"
           if significanti.fetch(0) == 0
               print "here\n"
               @current_type.endianness = :LITTLE_ENDIAN
           elsif significanti.fetch(significanti.size-1) == 0
             print "want big endian, last byte==> ", significanti.values_at(significanti.size-1), "\n"
               @current_type.endianness = :BIG_ENDIAN
           else 
               print "Warning: Byte order not supported ==>", "#{significanti}", "\n"
           end
        end
        print "Significances way byte order now ==> ", @current_type.endianness, "\n\n"
        return false # Already recursed
@ryanatball

This comment has been minimized.

Show comment
Hide comment
@ryanatball

ryanatball Mar 28, 2017

Member

Thanks for feedback. I'll take a look at this today.

Member

ryanatball commented Mar 28, 2017

Thanks for feedback. I'll take a look at this today.

@severn

This comment has been minimized.

Show comment
Hide comment
@severn

severn Mar 28, 2017

I made an attempt to provide a more comprehensive fix. I wrote 2 procedures ... but I (did a bad thing) and modified the gem directly... and now it doesn't seem to load, instead using a cached version (this is a guess). Funnily I got by by this way for all my other debugging until this last part... I had to move onto to something else and my previous band-aid allowed the XTCE to load and be mostly useful. But because I know I have some number of weird byte orders, these are not being caught by the band-aid approach above.

Anyway, these are just suggestions...

Here is the code:

  # XTCE big endian check
    def isBigEndian(ordering)
	    (0..ordering.size-2).each do |i|
      print("BE check ==> ", ordering.fetch(i), " vs ", ordering.fetch(i+1));
			if (ordering.fetch(i) != (ordering.fetch(i+1)+1))
		        return false
		   end
		   
	    end
	    
        return (ordering.fetch(ordering.size-1) == 0)
    end
    
     # XTCE little endian check
    def isLittleEndian(ordering)
	    (0..ordering.size-2).each do |i|
      print("LE check ==> ", ordering.fetch(i), " vs ", ordering.fetch(i+1));
			if ((ordering.fetch(i)+1) != ordering.fetch(i+1))
		        return false
		   end
		   
	    end
	    
        return (ordering.fetch(0) == 0)
    end

severn commented Mar 28, 2017

I made an attempt to provide a more comprehensive fix. I wrote 2 procedures ... but I (did a bad thing) and modified the gem directly... and now it doesn't seem to load, instead using a cached version (this is a guess). Funnily I got by by this way for all my other debugging until this last part... I had to move onto to something else and my previous band-aid allowed the XTCE to load and be mostly useful. But because I know I have some number of weird byte orders, these are not being caught by the band-aid approach above.

Anyway, these are just suggestions...

Here is the code:

  # XTCE big endian check
    def isBigEndian(ordering)
	    (0..ordering.size-2).each do |i|
      print("BE check ==> ", ordering.fetch(i), " vs ", ordering.fetch(i+1));
			if (ordering.fetch(i) != (ordering.fetch(i+1)+1))
		        return false
		   end
		   
	    end
	    
        return (ordering.fetch(ordering.size-1) == 0)
    end
    
     # XTCE little endian check
    def isLittleEndian(ordering)
	    (0..ordering.size-2).each do |i|
      print("LE check ==> ", ordering.fetch(i), " vs ", ordering.fetch(i+1));
			if ((ordering.fetch(i)+1) != ordering.fetch(i+1))
		        return false
		   end
		   
	    end
	    
        return (ordering.fetch(0) == 0)
    end

@ryanatball ryanatball added bug and removed possible bug labels Mar 28, 2017

ryanatball added a commit that referenced this issue Mar 28, 2017

@ryanatball ryanatball reopened this Mar 29, 2017

ryanatball added a commit that referenced this issue Mar 29, 2017

Merge pull request #415 from BallAerospace/warning_on_bad_xtce_byte_o…
…rder

closes #411 Add warning for bad xtce byte order lists and unit tests

@ryanatball ryanatball modified the milestone: v3.9.1 Mar 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment