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

Improves named IO pins #2794

Merged
merged 6 commits into from
Dec 27, 2023
Merged

Improves named IO pins #2794

merged 6 commits into from
Dec 27, 2023

Conversation

havardAasen
Copy link
Contributor

There is still some limitations to the named arrays. If both names_dout and names_din is set, they have to be the exact same size. The same is true for names_ain and names_aout. Currently, we do not verify this.

Marking this as draft to see if the solution is accepted, I also thought to update the man-page for the named pins names_dout, names_din and names_ain, names_aout.

There is also the issue with not sending in the max length to `count_names(), not sure if you want that in 2.9 as well @andypugh?

@andypugh
Copy link
Collaborator

With this patch num_dio or num_aio is set to the minimum of count(inputs) and count(outputs).
I feel it might be better to take the max(count(in), count(out) and then modify the check here: https://github.com/LinuxCNC/linuxcnc/blob/master/src/emc/motion/motion.c#L489C5-L489C5
to create default-named pins when it runs out of names.
Currently users can define names for lots of inputs (say) and then not have them be created unless they have an equal number of outputs.

@havardAasen
Copy link
Contributor Author

Are you thinking we can split up the input and output? Like 15 inputs and 5 outputs, and of those inputs 10 is named and 5 is default-named, with something similar with the outputs, but only 5 outputs? Input and output does not have same amount of pins.

Or is the intention that if I want to create 10 named inputs and 5 named outputs, It would actually generate 10 named inputs, 5 named outputs + 5 default-named outputs? Input and output has same amount of pins.

This last one would give a limitation of 32 named pins, not sure if this will become a problem.

@andypugh
Copy link
Collaborator

I was thinking of the latter, but may have missed a detail. Do the IOs of each type share an array with each being input or output (I would check, but it's late here, and probably later where you are). My (un-checked) impression was that they were separate but of equal size.

I am really keen to get 2.9.2 out (for reasons due to the Debian bugfix timescale) Do you think that the current partial fix is good enough for the moment?

@havardAasen
Copy link
Contributor Author

I was thinking of the latter, but may have missed a detail. Do the IOs of each type share an array with each being input or output (I would check, but it's late here, and probably later where you are). My (un-checked) impression was that they were separate but of equal size.

You might be correct , I based my assumption on these lines

#if (EMCMOT_MAX_DIO > 64) || (EMCMOT_MAX_AIO > 64)
#error A 64 bit bitmask is used in the planner. Don't increase these until that's fixed.
#endif

and since it combines input and output pins into the same variable I assumed the same register. But as you say, it would make more sense if they were separate.

I am really keen to get 2.9.2 out (for reasons due to the Debian bugfix timescale) Do you think that the current partial fix is good enough for the moment?

Not sure if I want to have any opinion on that, for 2.9.1

motmod num_misc_error=10 # Creates 10 error pins
motmod names_misc_errors=one,two # Creates 0 error pins

In it's current state, this has been flipped

motmod num_misc_error=10 # Creates 0 error pins
motmod names_misc_errors=one,two # Creates 2 error pins

The num_misc_error was the only one documented in 2.9.1 as well.

@andypugh
Copy link
Collaborator

As you spotted, I managed to commit an incomplete fix. I have now committed the fix that I intended (and tested).
I think that what you have here is better, but I think it needs more careful assessment.
The issue with the error pins was a clear bug, whereas the general improvement affects code that has (apparently) been working OK for some time. (Maybe not that long, I wasn't even aware that you could give names to the IO pins)

@havardAasen
Copy link
Contributor Author

As you spotted, I managed to commit an incomplete fix. I have now committed the fix that I intended (and tested). I think that what you have here is better, but I think it needs more careful assessment.

Are you thinking on which solution to go for?

The issue with the error pins was a clear bug, whereas the general improvement affects code that has (apparently) been working OK for some time. (Maybe not that long, I wasn't even aware that you could give names to the IO pins)

Not sure if you have looked it up, but linking in the discussion in #1349 and the initial pr #1352 for the named IO pins.

Now that @andypugh fixed the bug in 2.9 this could target master instead, and we might be a bit more flexible with the solution and scope.

I don't think it is to hard to have equal amount of pins, where some is named and some isn't, splitting them up is a bit more work, though not by much.

There is still some limitations to the named pin arrays. If both
`names_dout` and `names_din` is set, they have to be the exact same
size. The same is true for `names_ain` and `names_aout`.
We will now have equal IO pins, named and default-named pins.

With the command:
    motmod names_ain=one,two names_aout=one,two,three

It will be created three analog input pins:
    - motion.ain-one
    - motion.ain-two
    - motion-analog-in-00
and three analog output pins:
    - motion.aout-one
    - motion.aout-two
    - motion-aout-three
@havardAasen havardAasen changed the base branch from 2.9 to master December 23, 2023 13:19
@havardAasen havardAasen changed the title Fixes handling of named pin arrays Improves named IO pins Dec 23, 2023
@havardAasen
Copy link
Contributor Author

Rebased against master and added the necesarry documentation, the documentation for the error pins which is located in 2.9 is not included in this commit.

Don't like the global variables, but not sure how to solve it without moving everything into the same function.

@havardAasen havardAasen marked this pull request as ready for review December 23, 2023 13:23
@andypugh
Copy link
Collaborator

Related to this (but not an immediate change) is #2532

@andypugh andypugh merged commit 38bd03b into LinuxCNC:master Dec 27, 2023
11 checks passed
@havardAasen havardAasen deleted the pin-names branch March 31, 2024 21:37
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.

None yet

2 participants