Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Added support for ErgoDox with STM32 Microcontroller #5398
Added support for ErgoDox with STM32 Microcontroller #5398
Changes from 21 commits
a392fb4
079fbc4
304cd5b
c2b9529
06f9f2a
4927abe
23b445a
3c5251f
0365c92
cf99732
fd84782
1f1f23c
ee657d7
ec07114
222e791
082de2c
4378d6c
224637b
3438cad
35591b3
f7b501d
86db0c2
c4ec830
20ae9ce
6ef3d88
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@yiancar since I think you're more familiar with this, any issues with this?
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.
Is there anything else I should improve about this? Or is this good to merge?
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 actually have never checked what API the f1 uses. Just to make sure @awkannan any takes on this?
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'd prefer more general means of determining the i2c code to use rather than a specific MCU symbol definition, although ChibiOS doesn't exactly make it easy. Even if it was just the generic family definition instead (
STM32F1xx
) I'd be happy enough.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 haven't tested this i2c code on STM32F1xx.
I agree with @pelrun. There are also multiple i2c buses available to use on STM chips, so this still is not enough.
Until we figure out a good way to handle i2c bus selection and using family definition, I'd personally prefer to keep this in keyboard branches, etc. Just override i2c_master in your local keyboard folder. But I'm not the judge of that, I defer to QMK people to figure it out. The reason I say this though is because my RGB code is stuck in purgatory because it doesn't support every STM32 chip yet.
Also, as it stands, any non STM32F1xx chip would fall back to the F303 code, which is wrong on many chips. I like how we do it in eeprom, where we raise an error if no one has written the code for a specific family.
I also don't know how I feel about making tons of ifdefs, but I don't know a better alternative.
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 best solution would probably be to add an explicit symbol in the board definition and choose based on that. I was planning on bringing in to core the main board definitions that are floating around some of the keyboards, at which point it's fairly straightforward to keep them in 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.
I agree with @pelrun, however I didn't make it so that you have to declare this in board.h because I feel like that would break compatibility with a lot of existing code. I am thinking about either let the board.h define the config structure and initialization, thus they can both control the i2c version and which i2c bus to use. However, again, this would come at the cost of backwards compatibility. I initially just provided my own i2c_master file, but @drashna suggested to me that I should probably use the standard i2c_master. So I modified the i2c master. And honestly, I see the i2cmaster file the best to be more of a utility than configure the i2c bus.
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.
If this is an issue right now, then it should be fine to revert back to the separate files.
Though, I'm not fond of having multiple files, but it seems like this is the best option until we have a better way to handle this.