Skip to content

Add support for Unit CardKB2.#1

Merged
GOB52 merged 6 commits intodevelopfrom
feature/unit/cardkb2
Mar 31, 2026
Merged

Add support for Unit CardKB2.#1
GOB52 merged 6 commits intodevelopfrom
feature/unit/cardkb2

Conversation

@TinyuZhao
Copy link
Copy Markdown
Member

No description provided.

@imliubo imliubo requested a review from GOB52 March 24, 2026 01:58
@GOB52
Copy link
Copy Markdown
Collaborator

GOB52 commented Mar 24, 2026

@imliubo @TinyuZhao

When will CardKB2 be released?
If it’s urgent, I’ll prioritize the review process alongside the fixes we’re currently working on.

@TinyuZhao
Copy link
Copy Markdown
Member Author

@imliubo @TinyuZhao

When will CardKB2 be released? If it’s urgent, I’ll prioritize the review process alongside the fixes we’re currently working on.

CardKB2 is expected to be released next Friday.

@GOB52
Copy link
Copy Markdown
Collaborator

GOB52 commented Mar 24, 2026

CardKB2 is expected to be released next Friday.

OMG!


@TinyuZhao

Question about KEY index constants vs key_map alignment

The KEY_* constants in unit_CardKB2.hpp appear to be offset from the key_map[] array in unit_CardKB2.cpp.

key_map[] includes empty slots at indices 10, 22, and 33 (for physical gaps / modifier-only positions), but the KEY_* constants
are numbered sequentially without accounting for these:

Constant Value key_map[value] Expected
KEY_Q 10 {0,0,0,0} (empty) 'q' is at 11
KEY_W 11 'q' 'w' is at 12
KEY_AA 21 delete Aa is at 22
KEY_SYM 33 fn sym is at 34

This also affects character_map[], which uses these constants — e.g. character_map['"'] points to KEY_P (19), but " is
actually in key_map[28] (sym mode of 'h').

Additionally, update() uses hardcoded literals that match key_map indices rather than the KEY_* constants:

if (kidx == 34) {  // sym — but KEY_SYM is 33
if (kidx == 22) {  // caps — but KEY_AA is 21

Is this intentional (i.e. two separate indexing schemes), or should the KEY_* constants match the key_map indices (KEY_Q = 11, KEY_AA 
= 22, KEY_SYM = 34, etc.)?
                                            

@TinyuZhao
Copy link
Copy Markdown
Member Author

Thank you for reminding me; I’ve already made the changes.

@GOB52
Copy link
Copy Markdown
Collaborator

GOB52 commented Mar 24, 2026

@TinyuZhao

I gave a quick review of the latest version.


1. Debug printf in unit_Keyboard.cpp

printf("released_key:0x%02x\n", _released_key);

This is in the base UnitKeyboard::update(), so it affects UnitCardKB and UnitFacesQWERTY as well. Was this left in by accident?


2. character_map[] — possible misalignment with key_map

I checked character_map entries against key_map and found a few that don't seem to match:

Key index mismatches (3 entries):

Char ASCII character_map points to key_map[that][sym] Expected KEY (based on key_map)
" 34 KEY_P (20) = KEY_H (28) → "
- 45 KEY_H (28) " KEY_I (18) → -
_ 95 KEY_I (18) - KEY_U (17) → _

These look like a circular shift — could you double-check?

Mode bit mismatches (2 entries):

Char ASCII character_map mode key_map location
, 44 1+2 (normal+shift) key_map[40 (n)][2] — sym mode (should be 4)
. 46 1+2 (normal+shift) key_map[41 (m)][2] — sym mode (should be 4)

3. readHardwareType() — missing implementation?

Declared in unit_CardKB2.hpp but I don't see a definition in the cpp file. Is the implementation coming later, or should the declaration be removed for now?


4. Fn key handling in UART mode

In update(), the UART code path handles Sym and Caps (Aa) but I don't see any handling for Fn (KEY_FN=33). There's no path that reads key_map[][3] (fn mode), so it looks like direction keys (Fn+Z/D/X/C) and ESC (Fn+1) won't work over UART. Is Fn support planned for a later update?


5. Fn direction key values vs datasheet

I compared the SCHAR_* constants with the datasheet (V2) and the numeric values appear swapped pairwise:

Key Datasheet Code
Fn+D (Up) 180 181 (SCHAR_UP)
Fn+Z (Left) 181 180 (SCHAR_LEFT)
Fn+C (Right) 182 183 (SCHAR_RIGHT)
Fn+X (Down) 183 182 (SCHAR_DOWN)

The direction-to-key assignment is correct, but the numeric codes are swapped. Which is authoritative — the datasheet or the code?


6. Sym mode z/x/c — uppercase vs lowercase

The datasheet Sym table shows uppercase Z(0x5A), X(0x58), C(0x43), but key_map uses lowercase z(0x7A), x(0x78), c(0x63). Is this intentional?

@TinyuZhao
Copy link
Copy Markdown
Member Author

TinyuZhao commented Mar 24, 2026

Thank you for reviewing this.
Question 1: Just to make debugging easier, I'll remove this debug print.
Question 2: The issue does indeed exist; I double-checked the character_map and made the necessary changes.
Question 3: The firmware design only supports reading the firmware version; it cannot read the hardware type.
Question 4: The FN key is expected to function only in Bluetooth mode. Although it is possible to detect whether the FN key has been pressed in UART mode, I want it to behave consistently with I2C mode.
Question 5: There are errors in the data sheet; I’ll have a colleague correct them.
Question 6: In Bluetooth mode, pressing FN+Z/X/C (regardless of whether the Caps Lock key is pressed) sends a direction command, so I didn't make a distinction in the key_map.

IMG_8929

@TinyuZhao TinyuZhao force-pushed the feature/unit/cardkb2 branch from 56ec20d to fca95d2 Compare March 24, 2026 11:46
@GOB52
Copy link
Copy Markdown
Collaborator

GOB52 commented Mar 25, 2026

@TinyuZhao

Thanks for the fixes!

I'll merge this and take care of the remaining minor cleanup (removing the commented-out printf and the readHardwareType declaration) on my side. I'll also apply the standard M5Unit-* library template updates (platformio.ini, workflows, examples for NessoN1 etc.) before release.

Therefore, I would like you to send CardKB2 to me.

@TinyuZhao
Copy link
Copy Markdown
Member Author

Title

🤝 CardKB2 and other EOL products are scheduled to ship tomorrow .

@GOB52
Copy link
Copy Markdown
Collaborator

GOB52 commented Mar 25, 2026

@TinyuZhao
I forgot something important.
Please change the merge target from “main” to “develop.”

@TinyuZhao
Copy link
Copy Markdown
Member Author

But it seems I don't have permission to modify the merged target.

image image

Maybe I should close the PR and create a new one.

@TinyuZhao TinyuZhao changed the base branch from main to develop March 25, 2026 03:53
@TinyuZhao
Copy link
Copy Markdown
Member Author

@TinyuZhao I forgot something important. Please change the merge target from “main” to “develop.”

So that's where it was hidden.

What a strange interface design. I don't understand why I have to click "Change Title" first when changing the merge target.

image

@GOB52
Copy link
Copy Markdown
Collaborator

GOB52 commented Mar 25, 2026

I have granted you “Write” permissions. Please give it a try.

@TinyuZhao
Copy link
Copy Markdown
Member Author

I have granted you “Write” permissions. Please give it a try.

The merge target has been successfully changed

@GOB52 GOB52 merged commit 63dbb58 into develop Mar 31, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants