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 adsDriver and autoparamDriver #69

Merged

Conversation

Synthetica9
Copy link
Contributor

Add adsDriver (https://epics.cosylab.com/documentation/adsDriver/) and autoParamDriver (dependency, https://epics.cosylab.com/documentation/autoparamDriver)

I'm not sure what is meant with the mandatory varname attribute (fairly new to EPICS) so I'm not sure if I got those correct, would like review on that.

Added myself as maintainer for these two.

@Synthetica9
Copy link
Contributor Author

Note that this is a different library from #45

Copy link
Collaborator

@minijackson minijackson left a comment

Choose a reason for hiding this comment

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

Amazing, thanks a lot!

To answer your question about varname, it has to do with how EPICS resolves dependencies. For example, in the case adsDriver, to tell EPICS where autoparamDriver is, you put a line in configure/RELEASE:

AUTOPARAM=/path/to/autoparamdriver

varname is the name of the variable. The only technical constraint I know is that it must be unique per package.

Changes looks good, I only have minor comments. Could you split your changes into 2 commits, and run nix fmt? I also like to keep varname near pname and version.

Thanks again!

pkgs/epnix/support/adsDriver/default.nix Outdated Show resolved Hide resolved
pkgs/epnix/support/adsDriver/default.nix Outdated Show resolved Hide resolved
pkgs/epnix/support/autoparamDriver/default.nix Outdated Show resolved Hide resolved
@minijackson
Copy link
Collaborator

Oh, another question: do you think it would be possible to have an automated test for these packages?

@agaget
Copy link
Collaborator

agaget commented Apr 9, 2024

Oh, another question: do you think it would be possible to have an automated test for these packages?

And to do that if you want to dive in, @minijackson find this python lib if you need : https://pyads.readthedocs.io/en/latest/documentation/testserver.html

And example of ioc tests are here : https://github.com/epics-extensions/EPNix/tree/master/ioc/tests/support

I know that because I am supposed to do so for the other ADS driver :)

@Synthetica9
Copy link
Contributor Author

Could you split your changes into 2 commits, and run nix fmt?

History should be clean now

I also like to keep varname near pname and version.

Changed this as well

Oh, another question: do you think it would be possible to have an automated test for these packages?

While I really like the NixOS test driver (did quite a bit of work on it back in the day), I can't really justify this to my employer atm, so don't expect this in the short run.

@Synthetica9 Synthetica9 requested a review from minijackson April 15, 2024 13:39
Copy link
Collaborator

@minijackson minijackson left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@minijackson minijackson merged commit ae62f8c into epics-extensions:master Apr 22, 2024
2 checks passed
@Synthetica9
Copy link
Contributor Author

Synthetica9 commented Apr 22, 2024

Hmm the driver uses sum reads, and the testServer doesn't seem to support this, so this requires upstream additions to the test server.

(EDIT: talking about the NixOS test for this component)

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.

3 participants