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

AP_HAL_ChibiOS scripts: Fixed chibios_hwdef checking for defaults presence in one folder and using another #27955

Conversation

joshanne
Copy link
Contributor

defaults_path is relative to the folder the hwdef is in.
self.default_params_filepath is relative to the directory the script is run from.

The current implementation could result in attempting to load a defaults file from a location other than the expected one, or not actually loading a file when one is available to be loaded.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

Needs a patch to Tools/completion/zsh/_waf

Needs to remove --params from parser object

Currently breaks the waf --default-parameters option, which may not be acceptable. Will just out-and break waf building when --params is removed.

@joshanne
Copy link
Contributor Author

joshanne commented Sep 3, 2024

Needs to remove --params from parser object

This isn't modifying waf, this is modifying chibios_hwdef.py - waf can still be called with --default-parameters. Calling with --default-parameters ends up calling down through to generate_hwdef_h in chibios.py.

That function calls into chibios_hwdef.py through subprocess.call("python chibios_hwdef.py ..... --params ...) as is the case here.

This should not break the --default-parameters option, since we're just cleaning up what is actually being used.

If you've got an example board that breaks, shoot me the command. I've tested with say, CubeOrange with the defaults, cube orange with overridden defaults using this parameter.

@peterbarker
Copy link
Contributor

Ah, darn.

Sorry, I had missed the fact that args.params (a global) was being used from within the body of the script class.

I contend that the correct fix here is not to remove it from the constructor arguments for ChibiOSHWdef, but to stop using args.params within that method body.

As it currently stands, ChibiOHWDef is not going to fare well when instatiated from places ither than chibios_hwdef.py` - but this PR is going to make things worse rather than better.

@peterbarker
Copy link
Contributor

Past #27988 the use of args.params is the only remaining use of args within ChibiOSHWDef - and it would be good to kill its use in this PR....

@joshanne joshanne force-pushed the pr/fix-default-parameters-from-console branch 2 times, most recently from c870c0a to b3bb5df Compare September 3, 2024 07:09
@joshanne
Copy link
Contributor Author

joshanne commented Sep 4, 2024

This is breaking because the subprocess call turns an empty list into a string when constructing the command line.

I'm going to close this given I have a follow up PR that will effectively resolve this by removing the subprocess calls.

@joshanne joshanne closed this Sep 4, 2024
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