Skip to content
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

Misc #356

Merged
merged 14 commits into from Sep 9, 2019

Conversation

@coreyeng
Copy link
Member

coreyeng commented Sep 2, 2019

Hello. I've got some small, miscellaneous stuff that I've added locally. Most of these are pretty small additions that I've needed for esoteric purposes, but instead of adding them to my own apps, I've added them here as they might be useful for others.

Some things, like the subblock or plugin additions I only discovered after I'd already added these (I've had this branch for a while), but since they were already added and in use, I didn't feel the need to take them out.

The only one I'm not sure on is the change for loading the application. There's a problem I'm encountering that I really can't find the cause of, but it seems related to loading OrigenTesters multiple times and having the ActiveConcern's included block run more than once. This seems to happen when plugins use Origen.app(plugin) before the application has loaded completely and I've found that if I take out loading the application when a plugin is given, everything resolves without any ill-effects. I've still been able to generate test program collateral and run normally without this, so I'm proposing we just remove it unless someone with more knowledge on the subject can weigh in.

Everything else should be spec-ed or documented. I glossed over some of things which already had a solution, such as using sub_block groups (already in the docs) instead of searching based on a class name. A quick summary:

  • You can now use the sub_blocks method to retrieve all sub_blocks of a given class. For example, dut.sub_blocks(GPIO) will return all sub_blocks of that class. You can also give it an instance of that class as well, yielding the same result. These some specs that show examples of this.
  • Along the same lines, I've also had times where I've wondered if a given instance can be treated like a subblock. It can be a bit tricky since subblock hierarchies come into play. I added a origen_subblock? method to Object that'll return if a given instance inherits from either Model or Controller.
  • Added a 'default plugin' which can be set in the top-level app's configuration. A fresh application will have this plugin set automatically, but after any changes things resume as normal. If the user manually resets the current plugin, a session variable is set which will block the default plugin from ever taking effect again in this workspace. Added some specs for this too.
  • The previous fixes for the rendering issues from @redxeth do resolve them but the browser tools (I use Chrome but I'd imagine others as well) still flag the 'http' links. It doesn't really cause any problems, but they count as errors as far as the browser is concerned, clutters up the logs, and makes debugging a bit more of a cluster. So, I changed some other http links to https.
  • Subblocks that don't have a define file will crash when used to generate the model page. Although I think this is more due to incomplete sub_block instantiation, I still don't think it should crash if not provided. This change will just return the register description if a define_file is not present.
@coreyeng coreyeng requested a review from Origen-SDK/core Sep 2, 2019
coreyeng added 5 commits Sep 2, 2019
…page to 'Current and Default Plugins'. Moved the misc topics out of the plugins section and placed under the miscellaneous topics section.
…os-complement heleprs.
# 'hi'.origen_subblock? #=> false
# @see https://origen-sdk.org/origen/guides/models/defining/#Adding_Sub_Blocks
def origen_subblock?
self.class.ancestors.include?(Origen::Model) || self.class.ancestors.include?(Origen::Controller)

This comment has been minimized.

Copy link
@ginty

ginty Sep 3, 2019

Member

Could just be written as:

is_a?(Origen::Model) || is_a?(Origen::Controller)

This comment has been minimized.

Copy link
@coreyeng

coreyeng Sep 3, 2019

Author Member

Yes, that's much cleaner. I'll update this.

@ginty

This comment has been minimized.

Copy link
Member

ginty commented Sep 3, 2019

Lot's of good stuff in here!

I don't think the load_application should be removed. I don't know why it was causing a problem, but logically it is there to ensure that the app has been loaded so that its plugins have been identified and loaded before looking up the requested plugin.
It might work without it if you app already references Origen.app before looking up the plugin's app, but some may not.

On the multi-pin fetch feature, should it not return a PinCollection rather than a dumb array?

@coreyeng

This comment has been minimized.

Copy link
Member Author

coreyeng commented Sep 3, 2019

Thanks for the feedback!

I think references to Origen.app is part of the problem. My other workaround, is to swap out all calls to require "#{Origen.root(:origen_testers)}/lib/origen_testers/interface" with
require "#{Gem.loaded_specs['origen_testers'].full_gem_path}/lib/origen_testers/interface". Functionally, this should be the same and my flows seem to generate all the same. To me, it points to some problem with Origen.app during the boot process. Not really sure how to go about debugging that though as the deeper problem is with Active Concern's included block being run multiple times, where the solution from the web is just "don't run it multiple times".

As for the pins, I'll look into returningPinCollections instead. I've not done any kind of anonymous PinCollections before but it would make more sense to return a pin collection over a standard array.

…in retrieval to return a PinCollection instance
@ginty

This comment has been minimized.

Copy link
Member

ginty commented Sep 3, 2019

I think doing the initial loading via something like this is not really the way it should be done:
require "#{Origen.root(:origen_testers)}/lib/origen_testers/interface"

I think I saw similar issues in the past and decided that making references to apps/plugins/roots at the time when all that stuff was still being loaded was a bad idea.

The solution is that if you are overriding 3rd party code, like origen_testers in this example, then you should not do it in a file called lib/origen_testers/interface.rb, rather it should be in a file called lib/my_app/origen_testers/interface.rb.

Then there is no ambiguity for you to resolve by an absolute reference and in your app you can just have require "origen_testers/interface".

@coreyeng

This comment has been minimized.

Copy link
Member Author

coreyeng commented Sep 3, 2019

If the former require statement isn't correct, then that's fine. I can change it. In this particular case, I inherited this code, but since it seemed to work at some point (got checked in after all) I figured I'd try to keep it as it was and make Origen support it.

We're not actually overwriting something. We're trying to make an interface hierarchy for various testblocks, platforms and processes. It seemed that using the former syntax, Origen would require origen_testers/interface multiple times, resulting in the ActiveConcern error. When using the latter, Ruby only includes the file once, so the ActiveConcern listener is only registered once and everything is happy.

@coreyeng

This comment has been minimized.

Copy link
Member Author

coreyeng commented Sep 3, 2019

In any case, if the former isn't supported, I'll update our plugin accordingly. Since it seemed to work at some point, I went the route of making it work but I reinstated that call, so should be a non-issue, at least w.r.t. this PR.

@coreyeng

This comment has been minimized.

Copy link
Member Author

coreyeng commented Sep 5, 2019

@ginty, are you okay with merging this? I've put load_application back in and updated our offending plugin accordingly. It boots fine now.

@coreyeng coreyeng merged commit 1bbc146 into master Sep 9, 2019
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 73.714%
Details
@coreyeng coreyeng deleted the misc branch Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.