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

Update enum type names to match nidigital API in other ADEs #1330

Closed
sbethur opened this issue Mar 17, 2020 · 6 comments · Fixed by #1349
Closed

Update enum type names to match nidigital API in other ADEs #1330

sbethur opened this issue Mar 17, 2020 · 6 comments · Fixed by #1349

Comments

@sbethur
Copy link
Contributor

sbethur commented Mar 17, 2020

Description of issue

Some enum type names in nimi-python nidigital API (e.g. DigitalState - In LV it is Pin State) may not match other ADEs (like LV and .NET). For the sake of usability and maintenance we should update enum names in nimi-python to match other ADEs.

@sbethur
Copy link
Contributor Author

sbethur commented Mar 17, 2020

LabVIEW .NET Python (Current) Python (Proposed) Notes
PMU Aperture Time Units PpmuApertureTimeUnits 'ApertureTimeUnits' PPMUApertureTimeUnits LV is inconsistent about having "PPMU" prefix for PPMU-based enums. In Python we should consistenly add the" PPMU" prefix
Bit Order BitOrder 'BitOrder'    
Conditional Jump Trigger ID N/a 'ConditionalJumpTrigger' ConditionalJumpTriggerID  
Trigger Digital Edge - Edge DigitalEdge 'DigitalEdge'   Not going to try and make the name consistent with LV since the name in LV isn't good
Pin State PinState 'DigitalState' PinState  
Drive Format DriveFormat 'DriveEdgeSetFormat' DriveFormat  
History RAM Cycles To Acquire HistoryRamCycle 'HistoryRAMCyclesToAcquire'    
N/a HistoryRamTriggerType 'HistoryRAMTriggerType'    
Measurement Mode FrequencyMeasurementMode 'MeasurementMode'   This is unreleased enum. I propose we rename it to FrequencyMeasurementMode in all APIs
PPMU Current Limit Behavior PpmuCurrentLimitBehavior 'PPMUCurrentLimitBehavior'    
Measurement Type PpmuMeasurementType 'PPMUMeasurementType'    
Output Function PpmuOutputFunction 'PPMUOutputFunction'    
N/a N/a 'PatternOpcodeEvent'    
Selected Function SelectedFunction 'SelectedFunction'    
N/a N/a 'SequencerFlag'    
N/a N/a 'SequencerRegister'    
N/a N/a 'SessionState'    
Export Signal SignalType 'Signal' ExportSignal  
Site Result Type SiteResultType 'SiteResult' SiteResultType  
Data Mapping SourceDataMapping 'SourceMemoryDataMapping' DataMapping  
TDR Endpoint Termination TdrEndpointTermination 'TDREndpointTermination'    
Trigger Terminals N/a 'Terminal' TriggerTerminal  
Termination Mode TerminationMode 'TerminationMode'    
Time Set Edge Type TimeSetEdge 'TimeSetEdge' TimeSetEdgeType For this one and for SiteResultType, the "Type" suffix is redundant, but that is what it is in LV. Should we be consistent with LV or should we get rid of the "Type" suffix?
N/a TriggerType 'TriggerType'    
Software Trigger N/a SoftwareTrigger'    
Write Static Pin State N/a WriteStaticPinState'    

@sbethur
Copy link
Contributor Author

sbethur commented Mar 17, 2020

@marcoskirsch @mlopezleon

Please review my proposal above to update certain enum names.

@marcoskirsch
Copy link
Member

I like your proposals.
Is SessionState in the public API? I'm surprised by that enum.

@sbethur
Copy link
Contributor Author

sbethur commented Mar 18, 2020

SessionState is not in the public API. The list is for all enum types in internal metadata, across all levels of access.

I updated the internal metadata to remove SessionState from python metadata.

@mlopezleon
Copy link

My only comment is regarding the "type" suffix. I agree it is redundant, I would be ok removing it. If we do, what about TtriggerType?

@sbethur
Copy link
Contributor Author

sbethur commented Mar 18, 2020

Hmm, good question. Apart from TriggerType, there's also HistoryRAMTriggerType and PPMUMeasurementType. Looking at these, Type suffix is not redundant is all the cases. To be consistent across all enum names and with LV, I'll leave the suffix in.

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

Successfully merging a pull request may close this issue.

3 participants