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

Support both AVR and ps2avrGB targets for new_project script #2811

Merged
merged 11 commits into from May 10, 2018

Conversation

mechmerlin
Copy link
Contributor

Summary of Changes

The new project script now has a second mandatory argument for firmware type to allow specification of either avr or ps2avrgb. In the future we will have arm.

Usage

./new_project.sh mykeeb avr
./new_project.sh mykeeb ps2avrgb

Common template files are now stored in template/base, template/avr, and template/ps2avrgb

The ps2avrgb default templates was taken from the b.fake code.

Help

  • I'm not quite sure what to call the 2nd argument. FIRMWARE_TYPE still doesn't feel correct.
  • Are there any cases I've missed with my error checking?
  • Could use someone who is proficient in bash scripting to look over how I've written things.

@drashna
Copy link
Member

drashna commented Apr 26, 2018

Split keyboards would be a good addition here as well.

@mechmerlin
Copy link
Contributor Author

@drashna what??? Who uses those monstrosities? jk jk.

What's a good example I can use as the template? If I'm throwing split in the mix, FIRMWARE_TYPE makes even less sense. Any naming advice?

@drashna
Copy link
Member

drashna commented Apr 26, 2018

lol, me. And I know i'm a monster with meat hammers for fingers. :P

That said, the nyquist or viterbi may be a good one to use as a template. Or ... go for the good ol' lets split. The first two have fewer revisions, which is why I recommended them. (and "avr_split" may be a good way to refer to it)

Also, I'm sure you'll hate this too, but a chibiOS board would also be a good idea here. :)

Also "KEYBOARD_TYPE" may be a better name for the argument

@mechmerlin
Copy link
Contributor Author

I've heard some very unsavory remarks about the lets_split keyboard code. I'd be weary about putting that and any of the "similar" ones into a template. I don't think chibiOS stuff is as fleshed out as ps2avr or the avr stuff.

@drashna
Copy link
Member

drashna commented Apr 26, 2018

I haven't seen said comments, but yeah the code is a bit messy.

But we have a good number of split boards that use the lets split's code soo....

But as for the chibiOS, yeah.

@mechmerlin
Copy link
Contributor Author

For the time being, I'll just change the variable name. I'm not too convinced about the necessity of the other two targets yet. I'll do some digging in the let's split stuff and see if there's something that can be improved first before I make it into a template.

@drashna
Copy link
Member

drashna commented Apr 27, 2018

That's fine for now, I think.

Also, ErinCall was working on cleaning up the split code, and I think merging it into the main code, so that splits could be more easily maintained in the future. So if that gets finished and merged, it wouldn't be needed as a target. So, there is that.

@@ -2,8 +2,10 @@
# Script to make a new quantum project
# Jack Humbert 2015

if [ -z "$1" ]; then
echo "Usage: $0 <keyboard_name>"
if [ -z "$1" -o -z "$2" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

This should default to AVR so we don't break current behavior. I think you could do that by moving the KEYBOARD=$1 and KEYBOARD_TYPE=$2 lines above this, then doing something like:

if [ -z "$KEYBOARD" ]; then
   # Usage
    exit 1
elif [ -z "$KEYBOARD_TYPE" ]; then
  KEYBOARD_TYPE=avr
fi

@mechmerlin
Copy link
Contributor Author

Thanks skully!

KEYBOARD_TYPE=$2

if [ -z "$KEYBOARD" ]; then
# Usage
Copy link
Member

Choose a reason for hiding this comment

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

Sorry if I wasn't clear here. I meant that you should print the usage there, was just a bit lazy and didn't copy and paste that part. :)

@mechmerlin
Copy link
Contributor Author

Anything else needed for this to get merged in?

@drashna
Copy link
Member

drashna commented May 9, 2018

Don't think so. Just waiting on @skullydazed or @jackhumbert to merge.

@jackhumbert jackhumbert merged commit 5346cb2 into qmk:master May 10, 2018
@jackhumbert
Copy link
Member

Cool! Thanks :)

@krusli
Copy link
Contributor

krusli commented May 10, 2018

One thing, I haven't ported some of my PS2AVRGB matrix.c fixes back to the main PS2AVRGB folder from the JJ40's folder.

The original matrix.c implementation by Luiz was missing calls to matrix_init_quantum() and matrix_scan_quantum(), instead calling matrix_scan_user() directly, which also caused things like Tap Dance to break (#2162).

Should I open a PR to integrate my changes in?

@mechmerlin
Copy link
Contributor Author

@krusli I would greatly appreciate that.

@mechmerlin mechmerlin deleted the feature/newproject_targets branch May 19, 2018 14:50
carlpehrson pushed a commit to carlpehrson/qmk_firmware that referenced this pull request May 30, 2018
* Stopping point at creating targets for new_project script

* Add second argument for target

* Add the ps2avrgb target

* consider the case where the firmware type target is not valid

* fix template files to be more generic

* Code cleanup

* Change variable name to be more descriptive

* make avr the default

* forgot to put the template files in

* Take out useless comments

* add usage info
hauleth pushed a commit to hauleth/qmk_firmware that referenced this pull request Jan 24, 2019
* Stopping point at creating targets for new_project script

* Add second argument for target

* Add the ps2avrgb target

* consider the case where the firmware type target is not valid

* fix template files to be more generic

* Code cleanup

* Change variable name to be more descriptive

* make avr the default

* forgot to put the template files in

* Take out useless comments

* add usage info
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

5 participants