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

Add pins rep cap #1122

Merged
merged 3 commits into from
Nov 15, 2019
Merged

Add pins rep cap #1122

merged 3 commits into from
Nov 15, 2019

Conversation

texasaggie97-zz
Copy link
Contributor

@texasaggie97-zz texasaggie97-zz commented Nov 14, 2019

  • This contribution adheres to CONTRIBUTING.md.
  • I've updated CHANGELOG.md if applicable.
  • I've added tests applicable for this pull request

What does this Pull Request accomplish?

  • pins is now a repeated capability
    session.pins[['VCC','CLK']].ppmu_voltage_level = 4
    session.pins['VCC,CLK'].ppmu_voltage_level = 4

List issues fixed by this Pull Request below, if any.

What testing has been done?

  • Visual inspection of generated files

@codecov
Copy link

codecov bot commented Nov 14, 2019

Codecov Report

Merging #1122 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1122   +/-   ##
=======================================
  Coverage   89.99%   89.99%           
=======================================
  Files          20       20           
  Lines        3679     3679           
=======================================
  Hits         3311     3311           
  Misses        368      368

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a2c643...a17e6a7. Read the comment docs.

@marcoskirsch
Copy link
Member

marcoskirsch commented Nov 14, 2019

Please put some usage examples in the description.
Would this make adding a "sites" repeated capability redundant?

The HRAM functionality is site-based so we will need both, maybe.

@texasaggie97-zz
Copy link
Contributor Author

Usage (from issue) added.

From my understanding pins and sites are two different things so this is needed regardless of what we decide about sites. sites may need a new solution as it doesn't look like it is a normal repeated capability.

@sbethur
Copy link
Contributor

sbethur commented Nov 15, 2019

From my understanding pins and sites are two different things so this is needed regardless of what we decide about sites.

That's correct. We need pins to be a rep cap irrespective of what we do for sites. This is mainly so users can do

session.pins['VCC,CLK'].ppmu_voltage_level = 4

instead of

session.channels['VCC,CLK'].ppmu_voltage_level = 4

If any operation needs to be done on specific sites, with this change, you can do

session.pins['site0/VCC, site0/CLK, site1/VCC, site1/CLK'].

This is obviously not good in terms of usability. It's slightly better if users setup pin groups in their pin maps. Then they can do

session.pins['site0/PinGroupA, site1/PinGroupA'].

But this is still not ideal. It would nice to do

session.sites[0,1].pins['PinGroupA'].

We could still need to allow using siteN in pins rep cap, to allow users to do

session.pins['site0/VCC, site1/CLK'].   # This use-case is uncommon

@marcoskirsch
Copy link
Member

But this is still not ideal. It would nice to do
session.sites[0,1].pins['PinGroupA'].

This will require more generator work. #1093 is already open for this discussion.

sites may need a new solution as it doesn't look like it is a normal repeated capability.

Agreed. Let's have that discussion in #1111


.. code:: python

session.pins['0-2'].channel_enabled = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a good example. How difficult is it to do something nidigital specific?
#1125 created


session.pins['0-2'].channel_enabled = True

passes a string of :python:`'0, 1, 2'` to the set attribute function.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to stop documenting implementation details. The customer doesn't have to know or care how this ends up calling into the C API.

#1126 created

@marcoskirsch marcoskirsch merged commit 70f3540 into master Nov 15, 2019
@marcoskirsch marcoskirsch deleted the bug1093/add_pins_rep_cap branch November 15, 2019 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make nidigital methods that accept pinList argument easier to use
3 participants