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

Put fiducials in position file #621

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

elafontaine
Copy link

@elafontaine elafontaine commented Oct 27, 2022

As the global is set to use the "default" variant that we have set to be "kibom" type, the position files inherit the _mechanical filter which remove unuseful pads from the pick and place file. Fiduals are hence not in the position file normally if the _mechanical filter is applied.

see the generic filter explanation;
https://github.com/INTI-CMNB/KiBot#supported-filters
and the supported variant kibom value of filter (exclude_filter) ;
https://github.com/INTI-CMNB/KiBot#supported-variants

Fix issue #566

As the global is set to use the "default" variant that we have set to be "kibom" type, the position files inherit the _mechanical filter which remove unuseful pads from the pick and place file.  Fiduals are hence not in the position file normally if the _mechanical filter is applied.

see the generic filter explanation;
https://github.com/INTI-CMNB/KiBot#supported-filters
and the supported variant default value of filter (`exclude_filter`) ;
https://github.com/INTI-CMNB/KiBot#supported-variants
@elafontaine elafontaine marked this pull request as ready for review October 27, 2022 00:51
@G-Pereira
Copy link
Collaborator

Great approach @elafontaine.
@sphawes this solves #566 in a very neat and clean way and does not affect anything else so it can be merged

@elafontaine
Copy link
Author

@G-Pereira I'm wondering if I should be adding a new task to the CI jobs to ensure the file desired are present... And in this case, that it contains the FID component as well.

The reasoning I'm following is because someone changing the configuration may cause this problem to re-occur. I see the CI as "continuous imrpovement" not "continuous integration" :).

Would you be open to that ?

@elafontaine
Copy link
Author

You can merge this one right away, I would do the suggestion in another issue/PR :)

@elafontaine
Copy link
Author

No Update ?

@elafontaine
Copy link
Author

@sphawes @G-Pereira I would like to know if this is relevant or if I should close it.

I'm the kind of men to do one thing per project before moving on to the next thing for that project. and this has been done for almost over a year. Please let me know what you want to do.

@G-Pereira
Copy link
Collaborator

Hey, can you re-run the GitHub workflow? The previous one has expired.
https://github.com/elafontaine/lumenpnp/actions/runs/3338496971/jobs/5526227581
Once you do this I will discuss it with @sphawes

@elafontaine
Copy link
Author

@G-Pereira done and I've updated my branch through github

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.

2 participants