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.

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

[Keyboard] add aki27/cocot46plus #23892

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

markstos
Copy link
Contributor

@markstos markstos commented Jun 10, 2024

Adds the cocot46plus keyboard by aki27

I'm submitting the minimal bits I'm offering to help maintain, which is the RP2040 variant running directly on QMK.

I've tried to convert a maximal amount from the old config.h and rules.mk to keyboard.json.

I don't regularly work with QMK source code so there is likely room for improvement here.

Types of Changes

  • Keyboard (addition or update)

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

keyboards/aki27/cocot46plus/keyboard.json Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/keyboard.json Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/keyboard.json Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/keyboard.json Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/keyboard.json Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/keymaps/default/rules.mk Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/readme.md Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/readme.md Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/rules.mk Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/keyboard.json Show resolved Hide resolved
Comment on lines 38 to 57
enum cocot_keycodes {

COCOT_SAFE_RANGE = SAFE_RANGE,
CPI_SW,
SCRL_SW,
ROT_R15,
ROT_L15,
SCRL_MO,
SCRL_TO,
SCRL_IN,

};

#define CPI_SW QK_USER_0
#define SCRL_SW QK_USER_1
#define ROT_R15 QK_USER_2
#define ROT_L15 QK_USER_3
#define SCRL_MO QK_USER_4
#define SCRL_TO QK_USER_5
#define SCRL_IN QK_USER_6
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not use user keycodes at the keyboard level

Copy link
Contributor Author

@markstos markstos Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into doing this, but the keycodes are used cocot46plus.c. It defines process_record_kb which mentions them. Should that whole function be moved into the default keymap?

   if (keycode == CPI_SW && record->event.pressed) {
        cocot_config.cpi_idx = (cocot_config.cpi_idx + 1) % CPI_OPTION_SIZE;
        eeconfig_update_kb(cocot_config.raw);
        pointing_device_set_cpi(cpi_array[cocot_config.cpi_idx]);
    }

    if (keycode == SCRL_SW && record->event.pressed) {
        cocot_config.scrl_div = (cocot_config.scrl_div + 1) % SCRL_DIV_SIZE;
        eeconfig_update_kb(cocot_config.raw);
    }
    
    if (keycode == ROT_R15 && record->event.pressed) {
        cocot_config.rotation_angle = (cocot_config.rotation_angle + 1) % ANGLE_SIZE;
        eeconfig_update_kb(cocot_config.raw);
    }

    if (keycode == ROT_L15 && record->event.pressed) {
        cocot_config.rotation_angle = (ANGLE_SIZE + cocot_config.rotation_angle - 1) % ANGLE_SIZE;
        eeconfig_update_kb(cocot_config.raw);
    }

    if (keycode == SCRL_IN && record->event.pressed) {
        cocot_config.scrl_inv = - cocot_config.scrl_inv;
        eeconfig_update_kb(cocot_config.raw);
    }

    if (keycode == SCRL_TO && record->event.pressed) {
        { cocot_config.scrl_mode ^= 1; }
    }

    if (keycode == SCRL_MO) {
        { cocot_config.scrl_mode ^= 1; }
    }

    return true;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QK_KB_0 and such should be used here rather than QK_USER_0 and such.

keyboards/aki27/cocot46plus/cocot46plus.h Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/keyboard.json Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/matrix.c Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/layout-reference.md Outdated Show resolved Hide resolved
@markstos markstos force-pushed the add-aki26-cocot46plus-keyboard branch from 8274431 to 2ce51bb Compare June 11, 2024 12:53
@markstos
Copy link
Contributor Author

Thanks for the prompt and helpful feedback. I've applied a number of suggestions. I currently need to pivot to $job but have started getting some build failures for the firmware.

I can investigate more later, but suggestions welcome!

 | /usr/bin/avr-ld: /tmp/ccAEZNvn.ltrans0.ltrans.o: in function `.Loc.4948':
 | /home/mark/git/qmk_firmware/quantum/keyboard.c:420:(.text.startup.main+0xe4): undefined reference to `matrix_init'
 | /usr/bin/avr-ld: /tmp/ccAEZNvn.ltrans0.ltrans.o: in function `.Loc.5015':
 | /home/mark/git/qmk_firmware/quantum/bootmagic/bootmagic.c:65:(.text.startup.main+0x1fa): undefined reference to `matrix_scan'
 | /usr/bin/avr-ld: /home/mark/git/qmk_firmware/quantum/bootmagic/bootmagic.c:67:(.text.startup.main+0x210): undefined reference to `matrix_scan'
 | /usr/bin/avr-ld: /tmp/ccAEZNvn.ltrans0.ltrans.o: in function `.Loc.5033':
 | /home/mark/git/qmk_firmware/quantum/bootmagic/bootmagic.c:56:(.text.startup.main+0x216): undefined reference to `matrix_get_row'
 | /usr/bin/avr-ld: /tmp/ccAEZNvn.ltrans0.ltrans.o: in function `.Loc.5195':
 | /home/mark/git/qmk_firmware/quantum/keyboard.c:538:(.text.startup.main+0x49e): undefined reference to `matrix_scan'
 | /usr/bin/avr-ld: /tmp/ccAEZNvn.ltrans0.ltrans.o:/home/mark/git/qmk_firmware/quantum/keyboard.c:541:(.text.startup.main+0x4b6): undefined reference to `matrix_get_row'
 | /usr/bin/avr-ld: /tmp/ccAEZNvn.ltrans0.ltrans.o:/home/mark/git/qmk_firmware/quantum/keyboard.c:553:(.text.startup.main+0x4ce): undefined reference to `matrix_print'
 | /usr/bin/avr-ld: /tmp/ccAEZNvn.ltrans0.ltrans.o:/home/mark/git/qmk_firmware/quantum/keyboard.c:559:(.text.startup.main+0x4d6): undefined reference to `matrix_get_row'
 | /usr/bin/avr-ld: /tmp/ccAEZNvn.ltrans0.ltrans.o: in function `.Loc.5288':
 | /home/mark/git/qmk_firmware/platforms/suspend.c:45:(.text.startup.main+0x59e): undefined reference to `matrix_scan'
 | /usr/bin/avr-ld: /home/mark/git/qmk_firmware/platforms/suspend.c:48:(.text.startup.main+0x5a6): undefined reference to `matrix_get_row'
 | collect2: error: ld returned 1 exit status

keyboards/aki27/cocot46plus/cocot46plus.c Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/keyboard.json Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/keymaps/default/rules.mk Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/keyboard.json Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/cocot46plus.h Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/cocot46plus.c Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/matrix.c Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/matrix.c Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/matrix.c Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/matrix.c Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/matrix.c Outdated Show resolved Hide resolved
lib/chibios Outdated Show resolved Hide resolved
@markstos
Copy link
Contributor Author

I believe I've addressed the outstanding feedback and did a test flash of the firmware to the keyboard, but afterwards only the left halve and the pointing device register any key events. I'll work on reverting different parts of the changes until I get back to a working state. I expect it was changes related to the matrix.

keyboards/aki27/cocot46plus/cocot46plus.c Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/rules.mk Show resolved Hide resolved
keyboards/aki27/cocot46plus/matrix.c Show resolved Hide resolved
@markstos
Copy link
Contributor Author

Thanks for the help again. I've addressed the most recent suggestions and now the right half of the keyboard is working again-- 🚀-- but I noticed the OLED was no longer working. So I added back oled:true, but now the screen displays a random pattern of dots with "506" in the upper left corner. I'll go back through and revert the OLED-related refinements until it works again.

@markstos
Copy link
Contributor Author

I understand now what went wrong in the OLED refactoring. There were two issues with the attempt to get rid of snprint. The first was the number of characters printed didn't precisely match, the second was that the old code used an signed int to display an angle of "-30", but the new function, get_u8_str() expects an unsigned int.

This keyboard is challenged to have all of its features enabled on a Pro Micro on modern QMK. I've ordered the parts to switch mine over to an RP2040 with more storage. Then we could use snprint without complications.

I'll convert this a draft PR and re-open once I have something confirmed to work with the RP2040.

@markstos markstos marked this pull request as draft June 25, 2024 13:28
@markstos markstos marked this pull request as ready for review July 17, 2024 13:53
@markstos
Copy link
Contributor Author

I now have this keyboard working with RP2040 and am targeting that instead of Pro Micro. I was able to get an get an alternative to sprintf working, so I reverted to using that, which there is plenty of room for on the RP2040.

@markstos
Copy link
Contributor Author

I found the documentation of the GPX notation: https://docs.qmk.fm/platformdev_rp2040#pin-nomenclature

And I found the source code of the converter, but it doesn't show what the mappings to GPx notation are, but some other notation that ends in "U". (Below).

I found the pin-out for the Pro Micro: https://learn.sparkfun.com/tutorials/pro-micro--fio-v3-hookup-guide/hardware-overview-pro-micro

But this doesn't match either the left or right-hand side of what's in the converter file.

I'm stuck for now.

// Left side (front)
#define D3 0U
#define D2 1U
//      GND
//      GND
#define D1 2U
#define D0 3U
#define D4 4U
#define C6 5U
#define D7 6U
#define E6 7U
#define B4 8U
#define B5 9U

// Right side (front)
//      RAW
//      GND
//      RESET
//      VCC
#define F4 29U
#define F5 28U
#define F6 27U
#define F7 26U
#define B1 22U
#define B3 20U
#define B2 23U
#define B6 21U

// LEDs (Mapped to QT connector to avoid collisions with button/neopixel)
#define D5 17U
#define B0 16U

@yudai-nkt
Copy link

yudai-nkt commented Jul 22, 2024

I tried this PR yesterday, and the following commit seems to do the job mentioned in the review:

yudai-nkt/qmk_firmware@8ec47db

However, this introduces another error:

$ qmk compile -kb aki27/cocot46plus -km default
Ψ Compiling keymap with gmake -r -R -f builddefs/build_keyboard.mk -s KEYBOARD=aki27/cocot46plus KEYMAP=default KEYBOARD_FILESAFE=aki27_cocot46plus TARGET=aki27_cocot46plus_default INTERMEDIATE_OUTPUT=.build/obj_aki27_cocot46plus_default VERBOSE=false COLOR=true SILENT=false QMK_BIN="qmk"

[snip]

platforms/chibios/drivers/i2c_master.c: In function 'i2c_init':
././platforms/chibios/boards/QMK_PM2040/configs/config.h:17:26: error: 'D0' undeclared (first use in this function)
   17 | #    define I2C1_SCL_PIN D0
      |                          ^~

[snip]

 [ERRORS]
 |
 |
 |
gmake: *** [builddefs/common_rules.mk:373: .build/obj_aki27_cocot46plus_default/i2c_master.o] Error 1

The following patch fixes the above error and builds a functioning firmware, but I doubt this is an appropriate fix as it involves modification outside the keyboard directory.

diff --git a/platforms/chibios/boards/QMK_PM2040/configs/config.h b/platforms/chibios/boards/QMK_PM2040/configs/config.h
index f8b46b7fe4..87554f6b83 100644
--- a/platforms/chibios/boards/QMK_PM2040/configs/config.h
+++ b/platforms/chibios/boards/QMK_PM2040/configs/config.h
@@ -11,10 +11,10 @@
 #    define I2C_DRIVER I2CD1
 #endif
 #ifndef I2C1_SDA_PIN
-#    define I2C1_SDA_PIN D1
+#    define I2C1_SDA_PIN GP2
 #endif
 #ifndef I2C1_SCL_PIN
-#    define I2C1_SCL_PIN D0
+#    define I2C1_SCL_PIN GP3
 #endif

I asked a similar question at Discord but got no response so far.

@fauxpark
Copy link
Member

The U suffixed values are an intermediate step. Just look at the pinout on the hardware you have...

yudai-nkt referenced this pull request in yudai-nkt/qmk_firmware Jul 23, 2024
keyboards/aki27/cocot46plus/config.h Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/rules.mk Outdated Show resolved Hide resolved
@markstos
Copy link
Contributor Author

The encoder is working now, but the lights are now not.

I suspect the pin number for the ws2812 driver is not right.

@markstos
Copy link
Contributor Author

@yudai-nkt Are the lights working you with the latest changes on this branch? Could you check the pins for those?

@yudai-nkt
Copy link

yudai-nkt commented Aug 3, 2024

@markstos I've just sent a PR against your fork that fixes my silly mistake. Please take a look at markstos#1 when you have time!

@markstos
Copy link
Contributor Author

markstos commented Aug 4, 2024

All feedback is now been addressed and all features of the firmware have been tested to work as intended.

Thanks to everyone for the collaboration.

keyboards/aki27/cocot46plus/keyboard.json Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/cocot46plus.c Show resolved Hide resolved
keyboards/aki27/cocot46plus/cocot46plus.c Show resolved Hide resolved
keyboards/aki27/cocot46plus/keymaps/default/keymap.c Outdated Show resolved Hide resolved
markstos referenced this pull request in aki27kbd/vial-qmk Sep 4, 2024
along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

#include QMK_KEYBOARD_H
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include QMK_KEYBOARD_H
#include "cocot46plus.h"

Copy link
Contributor Author

@markstos markstos Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this proposed change was made, the keyboard no longer compiles:

Compiling: keyboards/aki27/cocot46plus/cocot46plus.c                                               keyboards/aki27/cocot46plus/cocot46plus.c: In function 'process_record_kb':
keyboards/aki27/cocot46plus/cocot46plus.c:135:20: error: 'CPI_SW' undeclared (first use in this function)
  135 |     if (keycode == CPI_SW && record->event.pressed) {
      |                    ^~~~~~
keyboards/aki27/cocot46plus/cocot46plus.c:135:20: note: each undeclared identifier is reported only once for each function it appears in
keyboards/aki27/cocot46plus/cocot46plus.c:141:20: error: 'SCRL_SW' undeclared (first use in this function)
  141 |     if (keycode == SCRL_SW && record->event.pressed) {
      |                    ^~~~~~~
keyboards/aki27/cocot46plus/cocot46plus.c:146:20: error: 'ROT_R15' undeclared (first use in this function)
  146 |     if (keycode == ROT_R15 && record->event.pressed) {
      |                    ^~~~~~~
keyboards/aki27/cocot46plus/cocot46plus.c:151:20: error: 'ROT_L15' undeclared (first use in this function)
  151 |     if (keycode == ROT_L15 && record->event.pressed) {
      |                    ^~~~~~~
keyboards/aki27/cocot46plus/cocot46plus.c:156:20: error: 'SCRL_IN' undeclared (first use in this function)
  156 |     if (keycode == SCRL_IN && record->event.pressed) {
      |                    ^~~~~~~
keyboards/aki27/cocot46plus/cocot46plus.c:161:20: error: 'SCRL_TO' undeclared (first use in this function)
  161 |     if (keycode == SCRL_TO && record->event.pressed) {
      |                    ^~~~~~~
keyboards/aki27/cocot46plus/cocot46plus.c:167:20: error: 'SCRL_MO' undeclared (first use in this function)
  167 |     if (keycode == SCRL_MO) {

keyboards/aki27/cocot46plus/cocot46plus.c Outdated Show resolved Hide resolved
keyboards/aki27/cocot46plus/matrix.c Show resolved Hide resolved
markstos and others added 25 commits November 13, 2024 20:22
Co-authored-by: Joel Challis <[email protected]>
Co-authored-by: jack <[email protected]>
After this round of refinements, the left half of keyboard and the
pointing device work, but no keycodes are emitted from the right half
at all.
But now it displays a random pattern instead of soemthing useful
Now the OLED display is no longer scrambled, but there are two new problems with it:

 1. The number "506" is printed over the design
 2. The screen never refreshes. As originally submitted, the OLED
    would update to reflect layer changes.
- The angle of -30 was not displaying correctly with unsigned type
- Explicitly cast a result to a double for consistent typing
- The code without snprintf didn't work. Revert to snprintf, but also
  switch to RP2040 which has room for it.
Encoder and lights are currently not working correctly.
Co-authored-by: Joel Challis <[email protected]>
There's room for it now in the RP2040 storage.
@markstos markstos force-pushed the add-aki26-cocot46plus-keyboard branch from 022ae27 to d382503 Compare November 14, 2024 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants