drivers: Implemented i2c bitbang driver

[purdeaandrei]

Description

This bitbang driver allows the user to use any two gpio pins for I2C communication.
To enable set the makefile variable I2C_DRIVER=bitbang

Implements fast mode and standard mode (using fast mode by default).
To select standard mode #define I2C_BITBANG_STANDARD_MODE in config.h

On STM32 devices with RT clock, this code will achieve very efficient timings, achieving above 390kHZ in fast mode.

On other devices i2c communication will be significantly slower.

On AVR devices 168kHz has can be achieved in fast mode.
On STM32 devices without an RT timer, only 25kHz can be achieved with the current implementation.

Please see here for logic analyzer plots: https://imgur.com/a/NnnSNx4

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: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• 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).

[sigprof]

Did not try to check the actual I2C implementation against the spec, but noticed some imperfections around.

[drashna]

Would prefer that this was implemented more like how other drivers are.

Eg:

qmk_firmware/common_features.mk

Lines 373 to 399 in f93ad79

	VALID_WS2812_DRIVER_TYPES := bitbang pwm spi i2c
	WS2812_DRIVER ?= bitbang
	ifeq ($(strip $(WS2812_DRIVER_REQUIRED)), yes)
	ifeq ($(filter $(WS2812_DRIVER),$(VALID_WS2812_DRIVER_TYPES)),)
	$(error WS2812_DRIVER="$(WS2812_DRIVER)" is not a valid WS2812 driver)
	endif
	OPT_DEFS += -DWS2812_DRIVER_$(strip $(shell echo $(WS2812_DRIVER) | tr '[:lower:]' '[:upper:]'))
	ifeq ($(strip $(WS2812_DRIVER)), bitbang)
	SRC += ws2812.c
	else
	SRC += ws2812_$(strip $(WS2812_DRIVER)).c
	ifeq ($(strip $(PLATFORM)), CHIBIOS)
	ifeq ($(strip $(WS2812_DRIVER)), pwm)
	OPT_DEFS += -DSTM32_DMA_REQUIRED=TRUE
	endif
	endif
	endif
	# add extra deps
	ifeq ($(strip $(WS2812_DRIVER)), i2c)
	QUANTUM_LIB_SRC += i2c_master.c
	endif
	endif

[purdeaandrei]

@drashna

Would prefer that this was implemented more like how other drivers are.

Could you clarify which parts of the code you're referring to?
I think there are a few specific issues with the i2c_master here that need to be considered:

• There are four implementations of i2c_master.c/i2c_master.h

  • avr vendor
  • chibios vendor
  • bitbang
  • atsam (I'm not currently supporting bitbang on atsam, so we can most likely ignore this)

• I can't just have i2c_master.c for vendor, and i2c_master_bitbang.c for bitbang because lots of makefile code does things like:

  else ifeq ($(strip $(EEPROM_DRIVER)), i2c)
    QUANTUM_LIB_SRC += i2c_master.c

So I would have to refactor all that somehow, if that's the road we want to go down on, but I would need some clarification as to what the refactor would look like.
That's why I used VPATH instead, and I put the files into different folders, so I don't have to refactor all that code that refers to i2c_master.c

• However in the meantime I also found some keyboards that refer to this file by full path. See commit
  with title "Build fixes of some keyboards that reference the i2c_master files by …" fixing these keyboards. I suppose having to do this change also makes this a breaking change, and I would have to retarget to develop, correct?

• The other issue with i2c_master is that the header files are not all cross-compatible: bitbang and avr-vendor support low level i2c routines, and timeout infinite. While chibios-vendor doesn't support low-level i2c routines, and has no infinite constant. Chibios i2c master also seems to have what I would consider a bug, where i2c_start()/i2c_stop() to do something that doesn't match the other drivers, that is:
  On avr and bitbang, i2c_start sends an i2c start condition, and i2c address. It's a low-level i2c access routine. On chibios instead it starts the peripheral. This function should not exist on Chibios, or should be called something else. On chibios this function is not needed cause all the other functions also start the peripheral.
  On avr and bitbang, i2c_stop sends a stop condition. On chibios i2c_stop() stops the peripheral itself. This function should not exist on chibios, or should be called something else.

I considered it out of scope of this PR to fix this bug, but the above provided example driver has a single .h file, and multiple .c file implementations. Merging those .h files would mean that either those idiosyncrasies have to be cleaned up first, or I would have to have #ifdefs in the common .h file.
I hope this PR won't be gated on having to solve those pre-existing idiosyncrasies first.

[sigprof]

I2C_TIMEOUT_INFINITE support could be added to the chibios driver, because the underlying functions support TIME_INFINITE; whether this feature is actually useful is another matter (at the moment there is no code in the repo that actually uses I2C_TIMEOUT_INFINITE, and I hope it would stay that way).

From the API standpoint, looks like there should be two layers of the I2C driver API:

1. Low-level API — i2c_start(), i2c_write(), i2c_read_ack(), i2c_read_nack(), i2c_stop(). This part of API is optional (the ChibiOS driver cannot provide it, and the current implementations of i2c_start() and i2c_stop() look like a mistake). However, there is code in the repo that actually uses these APIs (mostly keyboard-specific, but there is also drivers/oled/oled_driver.c, which implements i2c_transmit_P() internally, and drivers/qwiic/micro_oled.c, where the usage of i2c_start() seems to be unneeded, because then it just proceeds with using i2c_transmit() and other high-level APIs).
2. High-level API — i2c_transmit(), i2c_receive(), i2c_writeReg(), i2c_readReg() (maybe a more general function like i2c_transaction() that provides the full capabilities of i2cMasterTransmitTimeout() is missing here, in case some device needs more than a single address/command byte as provided by i2c_readReg()). This part of API needs to be provided by all drivers.

[purdeaandrei]

@sigprof I agree with all that, I absolutely would prefer if i2c driver api was all uniform, and I want to help make that a reality.
Just looked a bit at atsam i2c driver, and it's completely incompatible. There is a separate set of i2c1 and i2c2 functions. I think the api should evolve somehow to support multiple i2c ports, and maybe even hybrid drivers, i.e., use the vendor i2c driver on one port, and the bitbang one on another one where the vendor driver is not available.

However, the question is should this specific PR be gated on all that cleanup? I estimate the effort to get the i2c drivers uniform is at least twice as much as I spent developing this i2c bitbang driver, if not more, and I would like to get this in so my keyboards can use it.

[tzarc]

However, the question is should this specific PR be gated on all that cleanup? I estimate the effort to get the i2c drivers uniform is at least twice as much as I spent developing this i2c bitbang driver, if not more, and I would like to get this in so my keyboards can use it.

No, keep this separate. Much easier to review normalisation passes if that's all they include.

Multiple bus peripherals is already on my radar, I'd prefer if we discussed this before any work is embarked on.

[purdeaandrei]

@drashna Could you clarify, are you still waiting to see changes on this PR, given my comments after your review?

[drashna]

Ideally, the filename would be /drivers/(avr|chibios)/i2c_master_(name).c

But you're right, that would require chaning instances of i2c_master.c being added. In that case, I2C_MASTER_DRIVER_REQUIRED = yes may be more appropriate, and having a block that checks that, and then handles the appropriate driver.

Eg:

  else ifeq ($(strip $(EEPROM_DRIVER)), i2c)
    I2C_MASTER_DRIVER_REQUIRED = yes

And

 VALID_I2C_DRIVER_TYPES := bitbang vendor custom 
  
 I2C_MASTER_DRIVER ?= vendor 
 ifeq ($(strip $(I2C_MASTER_DRIVER_REQUIRED)), yes) 
     ifeq ($(filter $(I2C_MASTER_DRIVER),$(VALID_I2C_DRIVER_TYPES)),) 
         $(error I2C_MASTER_DRIVER="$(I2C_MASTER_DRIVER)" is not a valid WS2812 driver) 
     endif 
  
     OPT_DEFS += -DI2C_MASTER_DRIVER_$(strip $(shell echo $(I2C_MASTER_DRIVER) | tr '[:lower:]' '[:upper:]')) 
  
     ifeq ($(strip $(I2C_MASTER_DRIVER)), vendor) 
         SRC += i2c_master.c 
     else 
         SRC += i2c_master_$(strip $(I2C_MASTER_DRIVER)).c 
     endif
 endif

[stale]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[purdeaandrei]

I have rebased, and retargetted the changes to develop. (+Made it more correct for Xmega, and 3-byte PC AVRs.) (Not yet tested after rebase)
@drashna
The organization you mentioned above works well for WS2812, cause there is a single ws2812.h file.
The problem with i2c_master, is that each implementation has its own .h file.
We are relying on sort-of duck-typing to allow things that are only using high-level access functions, to work on different platforms.

Some of the possible solutions I see:

1. have a single i2c_master.h somewhere global, and have it include the implementation-specific header file using #if directives, so for example:

#if defined(I2C_MASTER_DRIVER_VENDOR)
#    ifdef __AVR__
#        include "../platforms/avr/drivers/i2c_master_avr.h";
          // Note, that the included file must either sit somewhere
          // not on VPATH, or it must have a different name,
          // it can't just be i2c_master.h
#    elif ....
          .....
#    endif
#elif defined(I2C_MASTER_DRIVER_BITBANG)
#    include ../drivers/i2c_bitbang/i2c_master_bitbang.h
#endif

This, implemented in combination with your suggestion on how to get i2c_master.c compiled in.

2. Like option 1) and try to extract the common parts into the global i2c_master.h, and have the i2c_master_$(implementation).h files only contain the implementation-specific differences.
   But unfortunately this won't really work, cause there are no common parts for all architectures. The samd one has a completely different interface. Maybe the i2c_master.h for avr and chibios could be in a folder that is not picked up by VPATH for samd. But that again means creating a new folder and adding it to VPATH.

3. Select i2c_master.c like you suggested, but select i2c_master.h with VPATH. This doesn't make sense in my opinion, if VPATH is involved, and we're moving i2c_master.h into it's own folder, then it's easiest to just use VPATH for everything.

4. Experiment as to the order of folders on VPATH, how that translates to include path, and which .h file is picked up first when multiple folders contain the same file name. Then maybe it's possible to not have to have the vendor-specific i2c_master.h files in their own dedicated folder, and have the i2c_bitbang override it, by being found first by the compiler. There's some risks here for example, that the order maybe won't be the same on all systems, or maybe unrelated makefile reorganisation could make relying on VPATH order fail.

5. Actually put in work to regularize all the i2c interfaces, so that a single global i2c_master.h will suffice for all drivers. However there was consensus above that it should not be done withing the scope of this PR.

6. Maintain the PR as it is for now, using VPATH.

I'm still leaning towards 6) but maybe there's a better 7th option I don't see. I'm happy to implement any solution to get this PR through, but we have to deal in some way with the i2c_master.h file too, not just with the .c file.

[purdeaandrei]

Oh, there's another issue: The linter PR tester is wanting to screw up my nicely formatted inline assembly to the point it's totally unreadable. Would it be possible to maintain my formatting of the inline assembly code? I'm happy to apply all the other C code changes the linter suggests, but the inline assembly is the way it is for readability, and what the linter formatter does makes no sense.

[drashna]

Oh, there's another issue: The linter PR tester is wanting to screw up my nicely formatted inline assembly to the point it's totally unreadable. Would it be possible to maintain my formatting of the inline assembly code? I'm happy to apply all the other C code changes the linter suggests, but the inline assembly is the way it is for readability, and what the linter formatter does makes no sense.

Ignore linter. :(

[sigprof]

You can try to surround some parts of the source with these special comments:

// clang-format off
    /* ... some code which would be screwed up by clang-format ... */
// clang-format on

[purdeaandrei]

Rebased again on top of develop, to fix issue with some files that always appeared modified due to line endings, and added small fix to performance regression that was due to not detecting correctly the AVR's ARCH. (i.e. XMEGA vs AVR8)

Here for fresh waves, tested on atmega32u4:
https://imgur.com/a/wAwpmEN

@sigprof Thanks, clang-format helped, there are no more issues with the linter.

[purdeaandrei]

@drashna I tried implementing option 1) from the message above

I've pushed it as an alternate PR, please let me know what you think which one you'd prefer, or if it should be completely different.
#14183

[purdeaandrei]

Rebased to fix conflict

[purdeaandrei]

Implemented 16-bit address register read/write functions, that newly landed in QMK develop,
and rebased to resolve conflicts.

[drashna]

Unfortunately, between the time that this PR was started, and now, a lot of things have changed.

If you could, reorganize the files so that the drive files are in /platforms/(arch)/drivers for each of these.

And would prefe that they're named how the other drivers are. Eg. i2c_master.c for the hardware implementation, and i2c_master_bitbang.c for this driver.

That way, the drivers are all over the place, and is more obvious which is which.

[purdeaandrei]

I've been keeping this, and the alternate version of this PR ( #14183 ) up to date with changes in develop, so I followed the move of the native i2c implementation to platforms (cause I had to do file moves/renames on the native implementation) , and I also implemented the 16-bit address register access functions. Please take a look at my alternate PR too, there I have renamed the i2c_master.c to i2c_master_bitbang.c. But renaming the file has some consequences namely that I had to touch a lot of keyboard Makefiles. Also in order to not move the native i2c implementation into a subfolder, I still had to rename some files, for example on the alternate implementation I had to rename:
platforms/avr/drivers/i2c_master.h -> platforms/avr/drivers/i2c_master_avr.h in order to make sure the _avr.h implementation is not picked up incorrectly when it's not in use. (And drivers/i2c_master.h conditionally includes one of the i2c_master_*.h variants.)
Could you let me know which version you consider is organized better, and as such better to base further work on it? I'd like to close the inferior one.

Regarding moving it into /platforms: I have a single i2c_master(_bitbang?).c for the bitbang implementation, currently it's in drivers/i2c_bitbang, if I were to split it up the single file into platforms/avr/drivers and platforms/chibios/drivers, I would be duplicating a lot of code (>50% of the main .c file). If /drivers/i2c_bitbang is not a good place for this, then can we keep it a single file, so maybe move it to a new folder /platforms/**common**/drivers or something like that?

[tzarc]

Can I just ask one simple question?

What does this PR solve?

There's no indication of intent in the description of this or in #14183. I2C is a multi-peripheral bus, and the pins for dedicated I2C hardware peripherals on AVR and ARM are both known at board design time.
If the intent here is to cater for poor design choices on a keyboard design, then surely this implementation can go directly into the keyboard in question? I'm not sure we want the core-level maintenance burden for a one-off problem.

If it ends up being used in multiple places, then perhaps it's worth revisiting the implementation in core then, but at this stage if it's for one keyboard I don't see the point.

[purdeaandrei]

@tzarc Yes, the intent is mostly to cater to poor design, and I believe this is valuable enough to include in core, let me argue:

1. The mistake is not always banal, and this can sometimes help out even experienced designers. For example in my case it wasn't the case that I connected the I2C eeprom to the wrong pins. Instead I was aware the chip I was using didn't have a built-in eeprom, and from the pre-design research I knew eeprom emulation was available for other chips, and implementing it was possible for my chip. (all true) I expected to implement eeprom emulation, however when I got the assembled boards back, and I started looking into actually writing the eemprom emulation, I realized how limited it would be on the chip I was using, mainly due to eeprom page layout (having to deal with large erase delays of no code execution, and/or having to run code from ram, or possibly being forced to use a custom bootloader, and/or customizing the flasher. There were various options, but none of them were acceptable), and I decided to instead build a solderable add-on board, in order to not throw away the existing boards. At this stage I did not have any pairs of native I2C-supported pins available anymore, and I was forced to use just GPIOs. I can imagine many other situations like this where a complete redesign is not an option, and you can't really blame the designer for not setting aside native i2c pins.
2. In my case this applies to 5 keyboards (all supported with the same controller pcb):
   • ibm/model_m_4th_gen/overnumpad_1xb
   • unicomp/classic_ultracl_post_2013/overnumpad_1xb
   • unicomp/classic_ultracl_pre_2013/overnumpad_1xb
   • unicomp/spacesaver_m_post_2013/overnumpad_1xb
   • unicomp/spacesaver_m_pre_2013/overnumpad_1xb
   • (with more on the way, for example Unicomp PC122 support will soon be implemented)
     If I were to include bitbang i2c in my keyboards, I would still need a core modification to allow replacing the default i2c driver with a custom one, so that the core i2c eeprom driver can call my i2c bitbang driver. Also I wouldn't want to copy-paste i2c bitbang into all my keyboards, I would still want it to be in a common place and referenced by my keyboards. Not sure how nice it would be to have it be in one of the keyboard folders, and have the other keyboards makefiles reference the file.
3. There is at least one another developer, that is known to have used this, even though it's not yet in core. See this comment from daskygit: drivers: Implemented i2c bitbang driver ALTERNATE #14183 (comment)
4. I have seen my PR mentioned at least 2 times is discord, as a solution to someone's existing problems
5. An example of a non-poor-design reason to have this available: If you want to communicate with two devices of the same type, for example if you want to drive two oled screens, and you're on a device that has only one pair of i2c pins, like atmega. Note that this specific usecase will not be supported immediately, since we don't have an api at the moment to address multiple i2c buses, but it's a possibility for the future.

[purdeaandrei]

Rebased to resolve conflicts