Refactor bluetooth support. Add support for Adafruit Bluefruit LE UART Friend.

[joshuarubin]

Description

This adds support for the Adafruit Bluefruit LE UART Friend.

It depends first on getting #8952 merged as that's what I used for testing. Once that is merged, I will rebase this PR.

This particular keyboard (BIOI G60 BLE) does not actually use the Adafruit module in whole, rather it just uses the MDBT40 (pdf) module that the Adafruit uses and is flashed (at the factory) with the Adafruit firmware. The QMK source for this keyboard, from the factory, is unreleased. I am in no way affiliated with BIOI and this is a clean room implementation.

The code for this came primarily from the Adafruit source code which in turn depends on parts from the Arduino source code. Additionally, some inspiration came from the, still unmerged, TMK Pull Request.

I intend to add support for the Adafruit Bluefruit LE SPI Friend to replace the existing support in QMK, but felt that this should be merged first.

This particular implementation only works with hardware serial support and without hardware (or software) flow control. Adding support for software serial and hardware flow control, since they were originally in the Adafruit source code, should be pretty easy. I omitted this support initially in order to facilitate simpler configuration in QMK (just a single #define required in most cases) and to keep the code size small (though it still compiles larger than I'd like). Further, the included support for hardware serial, without flow control, occupies only 2 pins on the MCU. This is obviously advantageous for keyboards where the number of available pins is always at a premium. The existing SPI support, for example, requires at least 5 pins.

QMK configuration to affect this:

• rules.mk: BLUETOOTH = AdafruitBLEUART (not so fond of this, open to suggestions, maybe AdafruitBLE_UART?
• #define AdafruitBleBaud 76800 is necessary for the G60 BLE because it's factory configured at this rate even though the Adafruit default is 9600.
• #define AdafruitBleBattery is supported, in that, it will enable the bluetooth le battery service, however, adding support for each keyboard will require a method of reading the analog battery voltage, somehow, and converting it to a percentage.
• #define OUTPUT_AUTO_ENABLE is supported and will automatically start and stop broadcasting depending on if it is plugged into usb or not. This means, for example, that your software keyboard will reappear on your phone when you plug the keyboard into your computer.

Items still on my TODO list:

• add support for SPI (likely in another PR)
• test that charging the keyboard (not connected to a computer) does not disconnect it from the host
• documentation

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).

[fauxpark]

It looks like this is adding a lot of duplicate and extra code; I would rather the existing BLE code be refactored to pull up the AT and SDEP stuff, so that the transport can be swapped out more easily.
EDIT: By the way, the SPI friend is already supported with the current implementation.

[joshuarubin]

I tried that first and it turned out quite badly, very messy and difficult to reason about (e.g. the event system for detecting connection and disconnection).

The adafruit base makes it very easy to switch transports. Maybe I wasn’t clear, but I’d like the existing BLE code to be deprecated in favor of integrating the SPI bits of the adafruit code. I just didn’t want to make an even larger change right off the bat. That’s why I suggested doing it in another, follow up, PR.

Further, I think you’ll find that the new code is far more feature full and easier to develop with. The uart’s bt capability here far surpasses what the existing spi code was capable of (e.g. real battery level indication, bt disconnection and reconnection when usb is plugged in and out, proper rgblight resumption and more). Adding the spi parts next will give it the same level of capability.

Once the existing spi bt code is removed (in favor of the adafruit stuff), the only redundant bits will be the small ‘serial_uart’ files and the standard ‘serial’ files which are also quite small. Both could be deprecated without a ton of work.

I have a board with spi too, so it will be no problem testing that.

[latincompass]

I have a board with spi too, so it will be no problem testing that.
A Great Job!Sir.

[joshuarubin]

I've refactored the bluetooth support under a single interface. One interface now supports the adafruit ble protocol (now over both uart and spi). The other, bluetooth_classic, for RN-42 and adafruit ezkey.

[fauxpark]

Why did you delete the SPI driver?

[joshuarubin]

Why did you delete the SPI driver?

I'm kind of driving blind here. Nothing else was using it. I can certainly add it back. I was just trying to keep everything clean.

The arduino libraries (currently under tmk_core/protocol/lufa/adafruit_ble and including FIFO, HardwareSerial, Print, SPI and Stream) could all be moved somewhere more general as well. They should work for all the avr mcus, but not chibios, for example.

I'm not trying to make any waves. I'd love to get this merged. I'm happy to do whatever the team needs to move forward.

[fauxpark]

I added spi_master because of the need for a generic API matching i2c_master: #8299
Currently, only the BLE code uses it, yes, but the idea is for anyone to leverage it, and to be able to drop in an implementation for ARM or AVR at will.

They should work for all the avr mcus, but not chibios, for example.

This is the main issue I have with including the Arduino libraries - they are very AVR-specific and the exact opposite of what spi_master is trying to achieve.

[joshuarubin]

OK, no worries. I added spi_master back.

This is the main issue I have with including the Arduino libraries - they are very AVR-specific and the exact opposite of what spi_master is trying to achieve.

The only "AVR-specific" parts I've added are:

• HardwareSerial
• SPI
• BluefruitLE_SPI (only minimally hardware specific since it references pins, but the main hardware specific work is done by SPI)

Each of those should be pretty easy to write for other mcus. Alternatively, now that I have things working, I can look into replacing the arduino spi backend with spi_master. However, I'd be really hesitant to rip out such well tested code.

[tzarc]

I'm kind of driving blind here. Nothing else was using it. I can certainly add it back. I was just trying to keep everything clean.

For clarity, spi_master provides a common interface for both AVR and ARM, and enables PRs such as #8780 to work with no code changes between each architecture. The Adafruit BLE SPI implementation was previously written for AVR, but was then switched over to spi_master in preparation for allowing use on ARM. It just happened to be the only supported device that had been merged at this point, as spi_master is a relatively recent development.

Additionally, there is support for other SPI devices in the development pipeline, and spi_master's removal effectively nullifies these abstractions we've put in place to be able to write code once, but support both architectures.

Each of those should be pretty easy to write for other mcus. Alternatively, now that I have things working, I can look into replacing the arduino spi backend with spi_master. However, I'd be really hesitant to rip out such well tested code.

Please do - choosing AVR over ARM is becoming less and less enticing, and with the addition of any new external peripherals I'd be reluctant to limit support just for AVR. We're slowly trying to convert existing peripherals to equivalent abstracted implementations in order to support both architectures transparently, so tying this even stronger to AVR feels like a regression.

[joshuarubin]

• I've got the spi version using spi_master now.
• I also updated the uart version to use common/uart.h, though I had to rewrite the c code for that and add a few funcs (the existing interface is unchanged, and it is still avr specific).
• I also moved all the bluetooth code into drivers/bluetooth.
• Further, I moved the bluetooth specific functionality from lufa.c into the generic host.c and keyboard.c funcs.
  Having bluetooth enabled does require a working version of outputselect which is not available for anything other than lufa. So if you try to enable bluetooth on another mcu, it will return an error because it is unimplemented. That said, bluetooth hasn't been supported on anything but lufa so far and the existing non-lufa builds aren't broken. Writing outputselect for chibios, for example, shouldn't be hard, but I don't know how it gets the usb device state.

[latincompass]

quote

• I've got the spi version using spi_master now.
• I also updated the uart version to use common/uart.h, though I had to rewrite the c code for that and add a few funcs (the existing interface is unchanged, and it is still avr specific).

Looking forward to your uart mode code.Sir great job!

[joshuarubin]

What can I do to keep this moving forward?

[skullydazed]

@latincompass please do not attempt to speak for the project.

[cmaier]

@joshuarubin Great work, I really appreciate what you did here. I'd really love to this merged!

I've just tested it and I maybe found an issue:
If you activate the the COMMAND feature and use MAGIC_KEY_LOCK it would lock the keyboard while using it via USB but not if you use it via Bluetooth.

I'm just able to configure QMK and unfortunately unable to develop QMK so I cannot provide a fix myself, sorry.

[joshuarubin]

@cmaier thanks for trying it out! I could have sworn that this was working over bt, but I just tested and you're right. I'll look into it. Thanks for reporting!

[joshuarubin]

OK, well I'm really stumped here.

The KC_LOCK key is being registered while disconnected from usb (e.g. if you lock the keyboard while on usb, then unplug it, press KC_LOCK again, plug it back into usb, it will now be unlocked).

It's funny that when OUTPUT_AUTO_ENABLE isn't defined you can lock, you'll see that keycodes are going to bluetooth but not usb, then unlock and they go to both.

The actual keycodes are being sent right next to each other (one to bt, the other to usb) in host.c, so I really have no idea how this is happening.

I'd appreciate anyone explaining how this is the case. Thank you!

[joshuarubin]

@cmaier I just pushed a change that fixes the keyboard lock over bluetooth. let me know if you have any issues with it.

[cmaier]

Working like a charm. 🎉
I've tested it connected via USB, disconnected from USB and it's working as I've expected it. 😃

Another thing I'm experiencing is this: Whenever I'm typing a little bit more than just a few words I can see that some KC's are being send multiple times while I'm connected via Bluetooth. That's can also happen while using copy/paste. Text ist then pasted several times as if I have pressed v multiiple times. (There was some pause here...) If itttttt's happening right now I just don't correct it. Ah yeah, there we go:

multiiple
itttttt's

Feels like there is something filling up and then it happens.

[joshuarubin]

I’ve experienced that too when there isn’t a solid connection to the Bluetooth module. Are you using a G60BLE? If so, I had to resolder the bt module in order to get things working well. A way you can test if this is the case is to try squeezing the bt module against the pcb and see if things improve. Seems like just bad qc.

[joshuarubin]

Rebased on latest master

[joshuarubin]

Somehow qmk/directors was removed from the reviewers list

[joshuarubin]

Ping. How can I keep this alive?

[fauxpark]

I think I'd prefer to go with a slightly different approach, that doesn't involve adding a huge chunk of Arduino-derived code. What I have so far looks similar to your bluetooth.[ch], however my goal is to also make it platform-agnostic, and it might even evolve into being a general wireless API, not just Bluetooth. I also need to tweak how output selection is done a bit, and I'm going to drop support for the Adafruit EZ-Key as the module is discontinued and there are no boards in the repo that use it.

[joshuarubin]

@fauxpark ok, is that code available somewhere? can I help get it across the line?

[drashna]

I think what @fauxpark is getting at, is less C++ code, more C code, and more hardware agnostic code (such as using the hooks for drivers/(arch)/serial.h`, so that this will work on both ARM and AVR boards.

[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.

[joshuarubin]

The C++ code is just the arduino stuff. I can rewrite that into C if necessary.

In addition to the arduino code though, there are new abstractions that would enable adding support for other bluetooth backends much easier. I think this would have a large benefit on the codebase.

Further, the support in this patch is simply a huge improvement over what's currently in master. Let's not let "perfect" be the enemy of "good enough".

I'd be happy to make changes and fix the conflicts if I could get some direction on how to proceed.

[stale]

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.