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.

Sign up for GitHub

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

Jump to bottom

Add customisable EEPROM driver selection #7274

Merged
merged 1 commit into from
Jan 24, 2020
Merged

Add customisable EEPROM driver selection #7274

merged 1 commit into from
Jan 24, 2020

Conversation

tzarc
Copy link
Member

@tzarc tzarc commented Nov 5, 2019

Description

Adds support for external I2C EEPROM devices to QMK.

AVR devices normally have onboard EEPROM storage -- this will override the existing functionality provided by avr-libc, re-routing the eeconfig functions to external storage instead.

Not all ARM devices have support for emulated flash-based EEPROM -- this overrides any existing implementation, also re-routing the eeconfig functions to external storage.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@drashna drashna requested a review from a team November 5, 2019 21:20
@tzarc
Copy link
Member Author

tzarc commented Nov 5, 2019

I’ve tested this with ARM hardware that didn’t have EEPROM support — oscilloscope decoder matches expected output.

I’ve also tested building this for handwired/onekey/promicro — verifying the elf with avr-objdump shows that the eeprom_xxxx_yyyy functions have been replaced by the new equivalents. Have not tested with actual hardware as yet.

I do note that I need to update documentation to specify which flag needs to be set in rules.mk for boards wishing to use this functionality.

@tzarc
Copy link
Member Author

tzarc commented Nov 5, 2019 •
edited
Loading

After a bit of discussion on Discord with @zvecr and @drashna:

  1. drop _i2c from path
  2. split up eeprom_i2c.h and make an eeprom_driver.h for erase/init, rest of the i2c defs stay
  3. look at introducing EEPROM_DRIVER = i2c for rules.mk usage, defaulting to internal as per existing pre-PR functionality
  4. long term -- look at standardising the interface for EEPROM functions, split out implementations based on platform (EEPROM_Erase vs. eeprom_i2c_erase etc.)

drashna
drashna reviewed Nov 6, 2019
View reviewed changes
docs/feature_i2c_eeprom.md Outdated Show resolved Hide resolved
@tzarc tzarc force-pushed the external_i2c_eeprom branch 5 times, most recently from 0165348 to fda554e Compare November 6, 2019 22:04
@tzarc
Copy link
Member Author

tzarc commented Nov 6, 2019

I think the restructuring should be okay now -- as per the docs updates we should be able to use EEPROM_DRIVER = xxxxx and the like now, defaulting to vendor which matches existing functionality.

Tested builds with vendor/i2c/transient -- all objdump output for both AVR and ARM matches what I'd expect, and it's been tested with ARM hardware talking over i2c.

tzarc
tzarc commented Nov 6, 2019
View reviewed changes
quantum/rgblight.c Outdated Show resolved Hide resolved
@tzarc tzarc force-pushed the external_i2c_eeprom branch from fda554e to 4753511 Compare November 7, 2019 02:30
@tzarc tzarc changed the title Add external I2C EEPROM driver Add customisable EEPROM driver selection Nov 7, 2019
@tzarc tzarc force-pushed the external_i2c_eeprom branch from 4753511 to 178bc0a Compare November 7, 2019 02:38
@tzarc tzarc force-pushed the external_i2c_eeprom branch from 178bc0a to 0046928 Compare November 7, 2019 02:48
@tzarc tzarc force-pushed the external_i2c_eeprom branch from 0046928 to 9b29bfe Compare November 7, 2019 02:55
@tzarc tzarc force-pushed the external_i2c_eeprom branch from 9b29bfe to 2f9fc33 Compare November 7, 2019 02:58
@drashna
Copy link
Member

drashna commented Nov 7, 2019

Just a heads up, you don't need to keep on squashing commits. We can do that when the PR is done.

However, either way is just fine.

@tzarc
Copy link
Member Author

tzarc commented Nov 7, 2019

Habit. People at work like "forgetting" to squash. 💯

@drashna
Copy link
Member

drashna commented Nov 8, 2019

lol. Not a problem, either way.

Also, does this work with the Massdrop boards?
Eg, the ATSAM based boards.

@tzarc
Copy link
Member Author

tzarc commented Nov 8, 2019

Seems not:
error: implicit declaration of function 'i2c_init'; did you mean 'i2c0_init'?
error: implicit declaration of function 'i2c_transmit'; did you mean 'i2c0_transmit'?
error: implicit declaration of function 'i2c_receive'

@drashna
Copy link
Member

drashna commented Nov 8, 2019

Okay, that doesn't surprise me.

While I would like it to be added, but I think that would be best as a separate PR, later on.

@tzarc
Copy link
Member Author

tzarc commented Jan 7, 2020

Anything else required at this point?

@drashna
Copy link
Member

drashna commented Jan 8, 2020

Anything else required at this point?

Only thing for me is testing on a Kinetis based board. Mine should arrive friday.

@tzarc tzarc force-pushed the external_i2c_eeprom branch 2 times, most recently from a24b139 to a478cf7 Compare January 18, 2020 10:23
@tzarc tzarc force-pushed the external_i2c_eeprom branch from a478cf7 to 4cd0b8f Compare January 18, 2020 23:58
@tzarc
Copy link
Member Author

tzarc commented Jan 19, 2020 •
edited
Loading

Identified (and fixed) an issue with LTO (surprise surprise) -- the buffer address and buffer size needed to be aligned to 4 bytes in order to get things to link correctly. The issue when enabled:

Linking: .build/handwired_tzarc_cyclone_default.elf                                                 [ERRORS]
|
| tmk_core/protocol/chibios/main.c: In function 'main':
| tmk_core/common/chibios/eeprom_teensy.c:525:18: error: iteration 4 invokes undefined behavior [-Werror=aggressive-loop-optimizations]
|      return buffer[offset];
|                   ^
| tmk_core/common/chibios/eeprom_teensy.c:546:11: note: within this loop
|      while (len--) {
|            ^
| lto1: all warnings being treated as errors
| lto-wrapper: fatal error: arm-none-eabi-gcc returned 1 exit status
| compilation terminated.
| /usr/lib/gcc/arm-none-eabi/6.3.1/../../../arm-none-eabi/bin/ld: error: lto-wrapper failed
| collect2: error: ld returned 1 exit status
|
tmk_core/rules.mk:298: recipe for target '.build/handwired_tzarc_cyclone_default.elf' failed
make[2]: *** [.build/handwired_tzarc_cyclone_default.elf] Error 1

@tzarc
Copy link
Member Author

tzarc commented Jan 21, 2020

Anything still pending, other than reviews?

@drashna
Copy link
Member

drashna commented Jan 22, 2020

So .... umm.... what happened to the EEPROM_I2C_24LC128 settings? :)

@tzarc
Copy link
Member Author

tzarc commented Jan 22, 2020

Reinstated.

@tzarc tzarc force-pushed the external_i2c_eeprom branch from b59fb6f to 316ef08 Compare January 22, 2020 16:15
@tzarc tzarc force-pushed the external_i2c_eeprom branch from 316ef08 to 98ca076 Compare January 24, 2020 01:11
@tzarc
Add customisable EEPROM driver selection (qmk#7274)
6472d6e 
- uprintf -> dprintf
- Fix atsam "vendor" eeprom.
- Bump Kinetis K20x to 64 bytes, too.
- Rollback Kinetis to 32 bytes as partitioning can only be done once. Add warning about changing the value.
- Change RAM-backed "fake" EEPROM implementations to match eeconfig's current usage.
- Add 24LC128 by request.
@tzarc tzarc force-pushed the external_i2c_eeprom branch from 98ca076 to 6472d6e Compare January 24, 2020 01:15
@tzarc tzarc merged commit d13ada1 into qmk:master Jan 24, 2020
@tzarc tzarc deleted the external_i2c_eeprom branch January 24, 2020 01:46
noroadsleft pushed a commit to noroadsleft/qmk_firmware that referenced this pull request Jan 24, 2020
@tzarc @noroadsleft
Add customisable EEPROM driver selection (qmk#7274)
27e0a12 
- uprintf -> dprintf
- Fix atsam "vendor" eeprom.
- Bump Kinetis K20x to 64 bytes, too.
- Rollback Kinetis to 32 bytes as partitioning can only be done once. Add warning about changing the value.
- Change RAM-backed "fake" EEPROM implementations to match eeconfig's current usage.
- Add 24LC128 by request.
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Feb 12, 2020
@tzarc @drashna
Add additional fixes to EEPROM driver selection (qmk#7274)
d10151c 
- uprintf -> dprintf
- Fix atsam "vendor" eeprom.
- Bump Kinetis K20x to 64 bytes, too.
- Rollback Kinetis to 32 bytes as partitioning can only be done once. Add warning about changing the value.
- Change RAM-backed "fake" EEPROM implementations to match eeconfig's current usage.
- Add 24LC128 by request.
fdidron added a commit to zsa/qmk_firmware that referenced this pull request Feb 14, 2020
@drashna @tzarc
@qmk-bot @fdidron
Add additional fixes to EEPROM driver selection (qmk#7274) (#266)
ca346bc 
* Add additional fixes to EEPROM driver selection (qmk#7274)

- uprintf -> dprintf
- Fix atsam "vendor" eeprom.
- Bump Kinetis K20x to 64 bytes, too.
- Rollback Kinetis to 32 bytes as partitioning can only be done once. Add warning about changing the value.
- Change RAM-backed "fake" EEPROM implementations to match eeconfig's current usage.
- Add 24LC128 by request.

* format code according to conventions [skip ci]

Co-authored-by: Nick Brassel <nick@tzarc.org>
Co-authored-by: QMK Bot <hello@qmk.fm>
Co-authored-by: Florian Didron <fdidron@users.noreply.github.com>
HokieGeek pushed a commit to HokieGeek/qmk_firmware that referenced this pull request Feb 21, 2020
@tzarc @HokieGeek
Add customisable EEPROM driver selection (qmk#7274)
599feca 
- uprintf -> dprintf
- Fix atsam "vendor" eeprom.
- Bump Kinetis K20x to 64 bytes, too.
- Rollback Kinetis to 32 bytes as partitioning can only be done once. Add warning about changing the value.
- Change RAM-backed "fake" EEPROM implementations to match eeconfig's current usage.
- Add 24LC128 by request.
fdidron added a commit to zsa/qmk_firmware that referenced this pull request Feb 26, 2020
@drashna @tzarc
@qmk-bot @fdidron
Add additional fixes to EEPROM driver selection (qmk#7274) (#266)
ebd4b1d 
* Add additional fixes to EEPROM driver selection (qmk#7274)

- uprintf -> dprintf
- Fix atsam "vendor" eeprom.
- Bump Kinetis K20x to 64 bytes, too.
- Rollback Kinetis to 32 bytes as partitioning can only be done once. Add warning about changing the value.
- Change RAM-backed "fake" EEPROM implementations to match eeconfig's current usage.
- Add 24LC128 by request.

* format code according to conventions [skip ci]

Co-authored-by: Nick Brassel <nick@tzarc.org>
Co-authored-by: QMK Bot <hello@qmk.fm>
Co-authored-by: Florian Didron <fdidron@users.noreply.github.com>
c0psrul3 pushed a commit to c0psrul3/qmk_firmware that referenced this pull request Mar 23, 2020
@tzarc @c0psrul3
Add customisable EEPROM driver selection (qmk#7274)
9d732b0 
- uprintf -> dprintf
- Fix atsam "vendor" eeprom.
- Bump Kinetis K20x to 64 bytes, too.
- Rollback Kinetis to 32 bytes as partitioning can only be done once. Add warning about changing the value.
- Change RAM-backed "fake" EEPROM implementations to match eeconfig's current usage.
- Add 24LC128 by request.
kylekuj pushed a commit to kylekuj/qmk_firmware that referenced this pull request Apr 21, 2020
@tzarc @kujawakk
Add customisable EEPROM driver selection (qmk#7274)
c662f3e 
- uprintf -> dprintf
- Fix atsam "vendor" eeprom.
- Bump Kinetis K20x to 64 bytes, too.
- Rollback Kinetis to 32 bytes as partitioning can only be done once. Add warning about changing the value.
- Change RAM-backed "fake" EEPROM implementations to match eeconfig's current usage.
- Add 24LC128 by request.
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
@tzarc
Add customisable EEPROM driver selection (qmk#7274)
f9a33fa 
- uprintf -> dprintf
- Fix atsam "vendor" eeprom.
- Bump Kinetis K20x to 64 bytes, too.
- Rollback Kinetis to 32 bytes as partitioning can only be done once. Add warning about changing the value.
- Change RAM-backed "fake" EEPROM implementations to match eeconfig's current usage.
- Add 24LC128 by request.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fauxpark fauxpark fauxpark left review comments

@drashna drashna drashna approved these changes

@zvecr zvecr zvecr approved these changes

Assignees
No one assigned
Labels
core enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tzarc @drashna @fauxpark @zvecr