Flow Comments for Documentation not working

[CodyBriscoe]

To get a sense of how the documentation generation worked with origen, I inserted comments into my flow using three different methods. The standard # comment right before a test method, using the cc method, and directly appending description: ["comment"] to the test method. Below is an example from my code of exactly what I tried, note that each of these snippets are in a loop in the source code.

Using the # comment method:

# "pcie_#{test_type}_#{lpbk_mode}_#{lpbk_path}_#{gen}_#{speed}_#{v}"
func "pcie_#{test_type}_#{lpbk_mode}_#{lpbk_path}_#{gen}_#{speed}".to_sym, cond: v

Using the cc method:

cc "pcie_setvar_#{v}"
hsio_setvar   cond: v

Appending description, as discussed in #220:

hsio_ifvm :static_ifvm_emphasis, amplitude: a, force: :mA0, state: drive, cond: v, description:
 ["static_ifvm_impedance_#{a}_mA0_#{drive}_#{v}"]

I didn't expect cc to work as I believe that is not yet implemented for flow file documentation if I understand correctly, and appending description hasn't been tested yet either. However I was expecting every test method using the # comment method to have output similar to the following:

Instead, only 3 of the 100 or so tests using the # comment had a description like above. Of those 3 tests, each were looped several times but only the first test in the loop had a description. There were many tests however that had no description at all, not even the first test in the loop. Below is a working description on the first test in a loop followed by the faulty second test:

Is there specific strict syntax/rules to using the # comment before a test or is the documentation generation currently bugged?

Thanks

[ginty]

Hi @CodyBriscoe,

Thanks for reporting this and looking forward to your further contributions to Origen.

This is expected due to the way that # comments are currently implemented. When this was first designed it wasn't really envisaged that a flow would be generated from a loop like that, rather the original goal of the flow API was it to be simple and declarative, such that it could itself function as readable documentation. i.e. they work fine in this kind of scenario:

# This is test 1
test :test1

# This is test 2
test :test2

# This is test 3
test :test3

If you did want to repeat sections of a flow, then it was expected that they would be split out to a sub flow file and be called like this:

[:min, :max].each do |vdd|
  import :my_sub_flow, vdd: vdd
end

When implemented like that I would expect all the tests to pick up the comments as written in :my_sub_flow.

Since then however, we have evolved such that engineers are using the full power of Ruby in their flow files, discarding the idea that they themselves would be documentation, and instead we generate specific documentation views of the flow.

So all in all, I would class this as an enhancement rather than a bug, and improving the # comment implementation to work here would certainly be worthwhile while adding support for variables in comments via the cc method.

Roughly, the problem is that the file is parsed for comments (with no Ruby evaluation applied, so loops don't generate multiple comment entries into this array) and placed into an array before the program generation starts.

As the flow is generated the comments are popped off the array due to the initial expectation that it would be very sequential in nature, this happens here: https://github.com/Origen-SDK/origen_testers/blob/5b89bf287b3d307bd6708c878666f3609a5fd3af/lib/origen_testers/flow.rb#L359

The array does contain the line number for each comment, so probably the above method could be improved to always just look up the comment by line number, preserving it in the array instead of popping (or shifting in this case) it off, and then it should have a good chance of working correctly within flow loops.

[CodyBriscoe]

Thanks @ginty,

I'll look into improving the documentation generation for loops.

However, what about all the tests that don't have any documentation generated, even for the first test in the loop?

For example my source code has 10 test methods if you throw away all the looping. So under the current implementation, if I add a # comment directly before each test there should be 10 tests with documentation, correct?

I altered all my tests to only use the # comment method and only 3 of the 10 tests are generating documentation correctly.

[ginty]

There should be 10 commented tests in that case indeed, can you post an example here of what your flow looks like?

[CodyBriscoe]

When generating the flow below and then compiling for web documentation, the only tests that have documentation generated for them are the log_number_variable test methods.

  Origen.top_level.pcie.test_flow(:dc)[:impedance].each do |v| # v = [vmin, vnom, vmax]
    group "pcie_static_data_logging_#{v}".to_sym do

      # setvar description
      hsio_setvar   cond: v    
      # static testvar description
      hsio_testvar  :static, cond: v      

      # data logging
      Origen.top_level.pcie.pin_polarity.each do |pol|     
        # rx_se_impedance_logvar description
        log_number_variable "rx_se_impedance_logvar_#{pol}".to_sym, cond: v      
      end # polarity 

      # tx_diff_impedance_logvar description
      log_number_variable :tx_diff_impedance_logvar, cond: v

    end # set/test/log sets per v
  end # v, ends set/test/log variables

  # AC flows
  Origen.top_level.pcie.test_flow(:ac).each do |test_type, voltages|
    group "pcie_AC_#{test_type}_group".to_sym do
      voltages.each do |v|
        group "pcie_AC_#{test_type}_#{v}_group".to_sym do 
          case test_type.to_s
          when 'compcode_read'
            # compcode_read description
            hsio_compcode_read cond: v
          when /^[i,e]nelb/
            # Loop through the PCIE modes/speeds
            group "pcie_AC_loopback_#{test_type}_#{v}_group".to_sym do
              Origen.top_level.hsio_interfaces(:pcie_inst).pcie.modes.each do |gen| # gen = [gen1, gen2, gen3]
                group "pcie_AC_loopback_#{test_type}_#{gen}_#{v}_group".to_sym do 
                  speed = Origen.top_level.hsio_interfaces(:pcie_inst).pcie.speed(gen, as: :Mbps) 
                  Origen.top_level.pcie.loopback_modes.each do |lpbk_mode| # lpbk_mode = [pcs, phy]
                    Origen.top_level.pcie.loopback_paths.each do |lpbk_path| # lpbk_path = [int, ext]
                      # loopback description
                      func "pcie_#{test_type}_#{lpbk_mode}_#{lpbk_path}_#{gen}_#{speed}".to_sym, cond: v
                    end # lpbk mode 
                  end # lpbk path
                end # loopback mode group
              end # gen
            end # loopback group
          end # test_type case
        end # voltage group
      end # voltages
    end # test type group
  end # end AC tests

[ginty]

That's pretty complex flow logic, definitely a lot more looping than I use in my flows at least (and where it seems to work ok). I think if you work on the comment assignment to make it more based on a line number lookup it might help.

Generally though, the current rules are that only comment lines that are immediately before a test will get attributed to it - i.e. you can't have blank lines between the comment and the test, and any comments that have flow control statements between the comment and the test won't be assigned:

# BAD:

# This test does blah
if dut.blah
  test :blah
end

# GOOD:

if dut.blah
  # This test does blah
  test :blah
end

[info-rchitect]

@ginty thanks for the help. Would a long term solution need to incorporate comments into the ATP model?

[ginty]

Comments already are in the ATP model, this is about the API to enter them, but ultimately they end up in the ATP.

[CodyBriscoe]

Hey @ginty,

Just to clarify, the web documentation description for a test method comes from options[:description] value that gets set here: https://github.com/Origen-SDK/origen_testers/blob/5b89bf287b3d307bd6708c878666f3609a5fd3af/lib/origen_testers/flow.rb#L359 correct?

I reduced my flow to something simple with only 4 tests generated:

Origen.top_level.pcie.ifvm_drive_states.each do |drive| 
  Origen.top_level.pcie.ifvm_current_force_values.each do |i| 
    # static_ifvm_impedance description
    hsio_ifvm :static_ifvm_impedance, amplitude: :db0, force: i, state: drive, cond: :vmin  
  end 
end 

I then put a binding.pry inside the add_description! method that I modifed to handle loops in 'origen_testers/lib/origen_testers/flow.rb', and checked the values getting set every time the method was entered and the output was correctly set for each test:

[9] pry(#<OrigenTesters::SmartestBasedTester::V93K::Flow>)> options[:source_line_number]
=> 12
[10] pry(#<OrigenTesters::SmartestBasedTester::V93K::Flow>)> options[:param_id]
=> :static_ifvm_impedance_db0_mA5_drive0_vmin
[11] pry(#<OrigenTesters::SmartestBasedTester::V93K::Flow>)> options[:description]
=> ["static_ifvm_impedance description"]

However, after running origen p and then origen web compile --remote --api, the documentation generated has no descriptions for any of the tests. Any ideas?

Thanks

[ginty]

Hi @CodyBriscoe, I'm on a training all of this week and I don't have the ability to try this right now, but I think if you put a breakpoint in the flow file you can call atp.raw (or maybe flow.atp.raw) to see the internal model (an abstract syntax tree) of the flow. Hopefully from that you will be able to see if you descriptions are making their way into the model, in which case the problem is a downstream rendering issue, or whether the issue is that the descriptions never make it into the model in the first place.

[CodyBriscoe]

Hey @ginty, I tried adding a breakpoint to my flow file and checking the atp.raw output in the debugger. It looks like the descriptions are not making it into the model.

[CodyBriscoe]

@ginty If it helps here is a snippet (all test's output was essentially identical) of the atp.raw output when checked against the simplified flow mentioned above:

[2] pry(#<Test::Interface>)> flow.atp.raw
=> s(:flow,
  s(:name, "pcie_mini_flow"),
  s(:volatile,
    s(:flag, "$Alarm")),
  s(:test,
    s(:object, <TestSuite: static_ifvm_impedance_db0_mA0_drive0_vmin>),
    s(:name, "static_ifvm_impedance_db0_mA0_drive0_vmin"),
    s(:number, 0),
    s(:id, "static_ifvm_impedance_db0_mA0_drive0_vmin"),
    s(:on_fail,
      s(:if_flag, "$Alarm",
        s(:render, "multi_bin;")))),
  s(:test,
    s(:object, <TestSuite: static_ifvm_impedance_db0_mA5_drive0_vmin>),
    s(:name, "static_ifvm_impedance_db0_mA5_drive0_vmin"),
    s(:number, 0),
    s(:id, "static_ifvm_impedance_db0_mA5_drive0_vmin"),
    s(:on_fail,
      s(:if_flag, "$Alarm",
        s(:render, "multi_bin;")))),

[ginty]

Hi @CodyBriscoe, I had a look and actually the description is not shown in the console by default, you have to retrieve it from <node>.description:

(byebug) atp.raw.find(:group).to_a.last
s(:test,
  s(:object, <TestSuite: program_ckbd_864CE8F>),
  s(:name, "PGM_CKBD"),
  s(:number, 1000),
  s(:on_fail,
    s(:set_result, "fail",
      s(:bin, 100),
      s(:softbin, 1100))))
(byebug) atp.raw.find(:group).to_a.last.description
["Instantiate tests via the", "interface"]

The reason for this is that the description is considered metadata rather than semantic information about the flow, you can read about that here: http://www.rubydoc.info/gems/ast/AST/Node

Bottom line though, you will have to dig a bit more to see if the descriptions are really present or not.

[CodyBriscoe]

@ginty, thanks for the info. Here's the output when I use the same atp calls in pry:

[3] pry(#<Test::Interface>):1> atp.raw.find(:group).to_a.last
=> s(:test,
  s(:object, <TestSuite: static_ifvm_impedance_db0_mA5_drive1_vmin>),
  s(:name, "static_ifvm_impedance_db0_mA5_drive1_vmin"),
  s(:number, 0),
  s(:id, "static_ifvm_impedance_db0_mA5_drive1_vmin"),
  s(:on_fail,
    s(:if_flag, "$Alarm",
      s(:render, "multi_bin;"))))
[4] pry(#<Test::Interface>):1> atp.raw.find(:group).to_a.last.description
=> nil

So the description seems to get lost somehow. To double check I put a breakpoint in add_description! again, and it seems that description is still getting set correctly:

[1] pry(#<OrigenTesters::SmartestBasedTester::V93K::Flow>)> options[:id]
=> :static_ifvm_impedance_db0_mA5_drive1_vmin
[2] pry(#<OrigenTesters::SmartestBasedTester::V93K::Flow>)> options[:description]
=> ["test description mA5 drive1"]

[info-rchitect]

@ginty any update here? has @CodyBriscoe done due diligence in his debug? this is an important issue we would like to close ASAP so we can document our test programs. thx!

[ginty]

I think I am not going to actively work on this any time soon I'm afraid, especially since it seems to work OK as far as I can see. You'll need to create a failing test case if you want me or someone else to look further, or dig in yourself.

[CodyBriscoe]

Hey @ginty, here's the test case I'm running:

# Test Flow Title
Flow.create( unique_test_names: nil, test_module: :pcie) do |options|
  # "test description #{i} #{drive}"
  hsio_ifvm :static_ifvm_impedance, amplitude: :db0, force: :mA0, state: :drive0, cond: :vmin # cond: v 
  binding.pry
end # end PCIE flow

This is running the original code, I do not have any of my modifications in place to adjust for comments over a looped test method or cc hijacking. The output of the atp.raw and its description from pry:

[1] pry(#<Test::Interface>)> atp.raw.to_a.last
=> s(:test,
  s(:object, <TestSuite: static_ifvm_impedance_db0_mA0_drive0_vmin>),
  s(:name, "static_ifvm_impedance_db0_mA0_drive0_vmin"),
  s(:number, 0),
  s(:id, "static_ifvm_impedance_db0_mA0_drive0_vmin"),
  s(:on_fail,
    s(:if_flag, "$Alarm",
      s(:render, "multi_bin;"))))
[2] pry(#<Test::Interface>)> atp.raw.to_a.last.description
=> nil

For completeness, here's the web documentation output.

Let me know if there's any other information I can provide or if you have any tips of where to start digging. Thanks

[ginty]

Sorry, but this is something on your application side, converting this to a simple functional test call in the OrigenTesters test suite:

# "test description #{i} #{drive}"                                                                                                                                            
func :static_ifvm_impedance, amplitude: :db0, force: :mA0, state: :drive0, cond: :vmin # cond: v                                                                              
debugger

shows that it works fine:

[6, 15] in /home/stephen/Code/github/origen_testers/program/prb1.rb
    6: 
    7:   self.resources_filename = 'prb1'
    8: 
    9:   # "test description #{i} #{drive}"
   10:   func :static_ifvm_impedance, amplitude: :db0, force: :mA0, state: :drive0, cond: :vmin # cond: v 
   11:   debugger
   12: 
=> 13:   import 'components/prb1_main'
   14: 
   15: end
(byebug) atp.raw
s(:flow,
  s(:name, "prb1"),
  s(:test,
    s(:object, <TestInstance: static_ifvm_impedance, Type: functional>),
    s(:name, "static_ifvm_impedance")))
(byebug) atp.raw.to_a.last
s(:test,
  s(:object, <TestInstance: static_ifvm_impedance, Type: functional>),
  s(:name, "static_ifvm_impedance"))
(byebug) atp.raw.to_a.last.description
["\"test description \#{i} \#{drive}\""]

You'll need to get @info-rchitect to help you if you can't work it out from there.

[CodyBriscoe]

@ginty I think I've traced the issue to the ATP modeling. It seems that when a test method is contained within an on_fail block, the ATP model adds that test method as a child rather than creating a new node.

Here's the test case flow I ran:

Flow.create do |options|
  # test one
  test :first_test, on_fail: -> {
    # test two
    test :second_test
  }
  binding.pry
end 

add_description! correctly puts the description into options[:description] which is then extracted by extract_meta! from https://github.com/Origen-SDK/atp/blob/28c3707c9db3049a65050bd693a9869d321607a6/lib/atp/flow.rb#L631

After that however, the node then creates a child when detecting that the test method has an on_fail option set, here: https://github.com/Origen-SDK/atp/blob/28c3707c9db3049a65050bd693a9869d321607a6/lib/atp/flow.rb#L356 which then calls extract_meta! again for the new child, overwriting the parent's description.

So when running the test case above, the flow.atp output shows:

[1] pry(#<Test::Interface>)> flow.atp.raw.to_a
=> [s(:name, "pcie_mini_flow"), s(:volatile,
  s(:flag, "$Alarm")), s(:test,
  s(:object, "first_test"),
  s(:on_fail,
    s(:test,
      s(:object, "second_test"))))]
[5] pry(#<Test::Interface>)> flow.atp.raw.to_a[2]
=> s(:test,
  s(:object, "first_test"),
  s(:on_fail,
    s(:test,
      s(:object, "second_test"))))
[6] pry(#<Test::Interface>)> flow.atp.raw.to_a[2].description
=> ["test two"]

This explains why the comments were being deleted in our application since our interface automatically adds an on_fail alarm handling, so the alarm's description, which was nil, was overwriting the test methods description.

Is there a way to preserve the parent node's description as well as the children's?

[ginty]

Hi @CodyBriscoe, good work tracking this down, thanks!

I've made the above updates to the master branch of ATP, to add a test case and fix for this. Instead of the description (and source line info) being stored locally in a variable, it is now stored in a stack which seems to now handle embedded tests correctly.

Could you try it and feedback if it resolves the problems you have been seeing?

[CodyBriscoe]

Looks like it's working great now with the ATP update, thanks!