-
Notifications
You must be signed in to change notification settings - Fork 18
BP355 support #20
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
base: master
Are you sure you want to change the base?
BP355 support #20
Conversation
jkroonza
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar enough with GTK to comment on the changes in the last commit, and the only other concern is the removals from interface.ui in the second commit.
Looks like vscode does funky things with newline characters on the last lines of files ... so please just double check those too.
But all in all I think this looks good, I've not performed and actual test run as I don't have relevant hardware at hand, my GNX unit is packed away quite far at the moment.
.gitignore
Outdated
| images/gdigi_icon.h | ||
| core | ||
| # VSCode files | ||
| .vscode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing or spurious newline on last line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
Regarding the interface.ui changes, the removed parts in the second commit were experimental UI edits mistakenly included in the first commit.
They weren’t meant to be part of this PR, so I’ve cleaned that up.
I also double-checked the newline characters at the end of the files (.gitignore). Everything now ends with a proper newline, and there are no unexpected changes introduced by VSCode.
Sorry for these issues, I am really new in git and github.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The day you don't learn something new is the day you may retire.
|
I don't think you pushed the update ... you probably require git push --force. |
|
I don't think I was really clear. |
|
But we still need the corrected version pushed, eg, git push --force. |
|
Thank you for your feedback! |
|
Ah, you fixed after the fact and added commits, rather than fixing the original commits by amending them. @desowin may feel differently, but I would prefer you fix the original commits rather than just slapping more commits on. This can be done with git rebase -i master and then using the fixup options if you do have existing commits like those fixing things. |
Refactor(gtk): replace deprecated GDK threads, mutex, GTK_STOCK Refactor(gtk): use gtk_builder for GUI, and resources for GUI file and icon file Add generated resources.c/h as ignored
feat(bp355): add support for BP355 device
28216e5 to
32dd09f
Compare
|
Okay, now I understand. I have rebased all the work into 2 clean commits: one for GTK evolutions, one for BP355 support. Now it should be better. Thanks a lot for the helpful review and guidance! I learned a lot from your feedback, not a day to go retired ;) |
jkroonza
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. No we wait and see if Tomasz is still active ... if not we'll have to figure out what we do from there.
Hi,
This pull request adds support for the Digitech BP355 bass multi-effects pedal.
The changes include:
🧪 Tested on:
Linux (Ubuntu 24.04.2 LTS)
Digitech BP355 connected via USB
🎸 The implementation follows the structure used for other Digitech devices and aims to maintain compatibility with existing models, although I don't have any to test.
Notes:
Please tag me or comment here if you'd like further details or collaboration.
Thanks for maintaining this project — it's a fantastic tool!
Ping @jkroonza, as you kindly offered to take a look.
Also saw @geedubess's fork, but did not get the actions I would need to give there (sorry, very beginner in github and linux dev);
which branch would you like to see the support of BP355 integrated?