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

Configured CLI to allow 'manual' option for layout. #535

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

Conversation

Andrew-Collins
Copy link

This places additional boards at specified locations. Also added 'destination' option to layout, to specify the destination of the source board for all layout types.

New options for the 'manual' option are 'boards' and 'locations'.
Both are lists, and could be changed to a dictionary or other format if that is preferred.

Tested the following:

  • Grid layout with destination (need to use --post 'origin:' to see the effect)
  • Manual layout with no additional boards
  • Manual layout with multiple additional boards
  • Manual layout but 'locations' and 'boards' options are different length (fails with message)
  • Dump to preset file

Let me know if you have any questions, more to come.

This places additional boards at specified locations.
Also added 'destination' option to layout, to specify the destination of
the source board for all layout types.
Copy link
Owner

@yaqwsx yaqwsx left a comment

Choose a reason for hiding this comment

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

Hi! Thank you for your proposal. I left some comments regarding the code in the comments. I am mainly concerned with inconsistent UI.

However, to give proper feedback and consider merging this, I need to understand the use of this layout type. What use cases should it cover? KiKit's CLI aims to cover 98% of penalization cases. The rest is designed to be handled either via using plugins or scripting, as we cannot handle all use-cases in the UI without it becoming too cluttered.

Have you tried covering your use case with a Layout plugin? If so, what limitations did the plugin impose?

I am aware that these features could be documented better, and we miss proper examples (#283). Therefore, any effort and PR in this regard are welcomed.

PS: If we agree on merging this, I will require adding documentation and at least basic system tests added.

kikit/panelize_ui_impl.py Outdated Show resolved Hide resolved
kikit/panelize_ui_sections.py Outdated Show resolved Hide resolved
kikit/panelize_ui_impl.py Outdated Show resolved Hide resolved
kikit/panelize_ui_impl.py Outdated Show resolved Hide resolved
kikit/panelize_ui_sections.py Outdated Show resolved Hide resolved
@Andrew-Collins
Copy link
Author

The application for this is primarily putting multiple different boards into a single panel. I originally wrote a Layout plugin to do this, but thought this is a generic enough task, and found that there was existing demand for this (#242). So I decided to roll it into the CLI.

If you are happy with this justification then I will address your concerns above, and then move onto docs and tests. Otherwise I'll close the PR and put a Layout plugin example somewhere for people to find.

@yaqwsx
Copy link
Owner

yaqwsx commented Mar 30, 2023

I am willing to merge such a feature. However, we have to find a clean way of UI for this. And I am still trying to figure out what's the right way.

One thing I came up with is: Let's always take a single source file. The source file could be either a single board or a board collection. Board collection would be either a path to a JSON file or directly JSON-encoded content that describes board mapping. The structure of the JSON can depend on the layout plugin used. For the manual layout, we could have something like this:

[
	{
		"board": "path/to/board.kicad_pcb",
        "source": {
			"tolerance": "10mm"
		}
        "pos": ["100mm", "20mm"]
	}
	{
		"source": "path/to/another/board.kicad_pcb",
        "pos": ["100mm", "20mm"]
	}

The semantics is as follows:

  • we expect a list of objects.
  • The objects must have board (path to board) and pos (target coordinates) attributes.
  • Optionally, the user can specify the attribute source that will be merged with the source preset specified via --source. So, you can just override the source specification for the particular board. This is necessary if you want to extract multiple boards from a single bard.
  • the position attributes should respect the common KiKit conventions for distance specification.

What do you think about this proposal? I think with this feature implemented, we could also support #533.

@Andrew-Collins
Copy link
Author

Andrew-Collins commented Mar 30, 2023

That sounds good to me, I'll get onto it hopefully today.
I'll use the following json template, as it seems closer to what you described:

[
	{
		"board": "path/to/board.kicad_pcb",
                "source": {
		        "tolerance": "10mm"
		},
                "pos": ["100mm", "20mm"]
	},
	{
	        "board": "path/to/another/board.kicad_pcb",
                "pos": ["100mm", "20mm"]
	}
]

define how to source and place a board in the panel.
values.append(Board(value, ["0mm","0mm"]))
else:
json_str = value
s = Section()
Copy link
Author

Choose a reason for hiding this comment

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

Using Section object in order to not rewrite the conversion code.
Might be cleaner to move that code out into it's own function?

@@ -138,8 +177,7 @@ def fun(ctx, args, incomplete):
return fun

@click.command()
@click.argument("input", type=click.Path(dir_okay=False),
**addCompatibleShellCompletion(pathCompletion(".kicad_pcb")))
@click.argument("input", type=Input())
Copy link
Author

Choose a reason for hiding this comment

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

Unsure what to do about shell completion

@@ -252,7 +252,7 @@ def buildLayout(preset, panel, sourceBoard, sourceArea):
hbonefirst=layout["hbonefirst"],
vbonefirst=layout["vbonefirst"])
substrates = panel.makeGrid(
boardfile=sourceBoard, sourceArea=sourceArea,
boardfile=boardList[0].path, sourceArea=sourceArea,
Copy link
Author

Choose a reason for hiding this comment

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

Just uses the first board in the list.
Would you rather it error out if there are multiple boards in the list?
Could even make a grid for each board in the list?

@Andrew-Collins
Copy link
Author

@yaqwsx apologies for the bump, didn't mean to ping you.

@snhobbs
Copy link

snhobbs commented Nov 7, 2023

What's the next steps for this? I started putting together a similar separate CLI that does the same thing with a different input method: https://github.com/snhobbs/kikit-multipanel. Is it better as a separate tool or integrated into the existing CLI?

@yaqwsx
Copy link
Owner

yaqwsx commented Nov 7, 2023

Hi! Andrew put good arguments, so I want this feature to be merged into KiKit. I am postponing merging this, as there two upcoming tasks on KiKit roadmap, that will make this feature more streamlined and better working:

  • the first one is internal refactoring where the goal is to remove all the "feature" methods from the Panel class. At the moment, the panel class is too long, hard to maintain and to extend. Instead, I want individual features to be in separate files. This will have (I hope a good!) impact on scripting. It will help us to fix Make KiKit more scripting friendly #283.
  • the second is the change of the geometry engine. At the moment we reach botch, functionality and performance limitations of Shapely and I would like to switch to cavalier_contnours. With this change, we can rework the partition line building algorithm that should end up working better for arbitrary layouts.

@snhobbs
Copy link

snhobbs commented Nov 7, 2023

Great! Can these be made into issues with a bit of background? I'd love to get into them so this can be officially added in!

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