add_pin_groups does not work as expected for a mix of individual pins and pin groups

[Dieleman]

For pin definition:
add_pins :target, size: 2, direction: :input, reset: :drive_lo
add_pin :nadd, direction: :input, reset: :drive_lo
add_pin :ee_n, direction: :input, reset: :drive_hi
add_pin :oscx, direction: :input, reset: :drive_lo
add_pin :prech, direction: :input, reset:

add_pin_group :kalos_all_pins, :nadd, :ee_n, :oscx, :prech
Correctly gives $dut.pins(:kalos_all_pins).size => 4

add_pin_group :kalos_all_pins, :nadd, :ee_n, :oscx, :prech, :target
However gives $dut.pins(:kalos_all_pins).size => 2

So adding another pingroup (:target) somehow resets the master group

[welguisz]

@Dieleman ... I will be looking at this and will give you an update in about 2 hours.

[ginty]

Nice one @welguisz, you may want to use this test case I made which shows the problem:

    it "pins groups can be composed of standalone pins and other groups" do
      $dut.add_pin(:tdi)
      $dut.add_pin(:tdo)
      $dut.add_pin(:tclk)
      $dut.add_pin(:tms)
      $dut.add_pins(:porta, size: 8)
      $dut.add_pins(:portb, size: 16)

      $dut.pins.size.should == 28
      $dut.add_pin_group :jtag, :tdi, :tdo, :tclk, :tms
      $dut.add_pin_group :ports, :porta, :portb
      $dut.add_pin_group :all, :tdi, :tdo, :tclk, :tms, :porta, :portb
      $dut.add_pin_group :all2, :jtag, :ports

      $dut.pins(:jtag).size.should == 4
      $dut.pins(:ports).size.should == 24
      $dut.pins(:all).size.should == 28
      $dut.pins(:all2).size.should == 28
    end

[welguisz]

@ginty ... Thanks for the code .. Seeing the error right now

$dut.pins(:all).size.should == 28
# Currently returning 24
`$dut.pins(:all2).size.should == 28
#  Currently return 28

This means that this test is a good test since it is showing that has an error before any fix is done.

[welguisz]

I found where the bug is at:

   def add_pin_group(id, *pins, &_block)
      if pins.last.is_a?(Hash)
        options = pins.pop
      else
        options = {}
      end
      # check if this is a pin group alias
      found = false
      group = nil
      unless options[:pins_only] == true
        pins.each do |i|
          if pin_groups.include?(i)
            group = add_pin_group_alias(id, i, options)
            found = true  #THIS IS THE BUG (PART 1)
          end
        end
      end
      unless found # not a pin group alias  ... THIS IS THE BUG (PART 2)
        group = Origen.pin_bank.find_or_create_pin_group(id, self, options)
        if block_given?
          yield group
        else
          if options[:endian] == :little
            pins.each { |pin| group.add_pin(pin, options) }
          else
            pins.reverse_each { |pin| group.add_pin(pin, options) }
          end
        end
      end
      group
      # Origen.pin_bank.add_pin_group(group, self, {:pins_exist => true}.merge(options))
    end

For PART1, I think the best action is to change it to
pins.remove(i)
and PART2, I think it should be
unless pins.empty?

[welguisz]

Just pushed the fix and started a pull Request. @ginty when Coveralls finishes, can you review and merge if everything looks correct.

[welguisz]

@Dieleman I just release origen 0.7.43 that has this fixed incorporated in it. If everything works, please let us know that this is resolved and we can close this issue.

Thanks

[Dieleman]

@welguisz Thanks, I tried it and this seems to fix the issue indeed. For me the issue can be closed.