meshtastic / firmware

Meshtastic device firmware
https://meshtastic.org
GNU General Public License v3.0
2.97k stars 711 forks source link

Adding support for Chatter keypad #4022

Open Gnu-Bricoleur opened 3 weeks ago

Gnu-Bricoleur commented 3 weeks ago

This PR add support for the Chatter keypad (see https://github.com/meshtastic/firmware/issues/2896) I believe it supports all the feature we could want from this keypad (arrows, select, cancel, upper case, lower case, numbers, punctuation, ...) I don't think there's any bug left. I tried to respect as much as possible the style and conventions of the rest of the code and it should be fairly easy to extend my code to support other serial keypads or other layouts. Please let me know if you have any suggestion to improve on this PR (changing some variable names, ...) I have one question, all my tests were done on a Chatter V1 so I can confirm the Chatter V2 build is fully compatible with the Chatter V1 HW, shouldn't we rename the build to just Chatter ?

CLAassistant commented 3 weeks ago

CLA assistant check
All committers have signed the CLA.

CLAassistant commented 3 weeks ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


s.migaud seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

GPSFan commented 3 weeks ago

Along with the code changes to implement the keyboard there need to be some documentation additions that detail the keyboard layout, operation and "known problems". As for Chatter 2 vs Chatter, I am open to either, but there seem to be more Chatter 2's out there in the wild, so IMHO we should stay with Chatter 2 and document that it is compatible with V1. It would also be nice to be able to map some canned messages to hot keys.

eureekasigns commented 3 weeks ago

Agree on layout/map. Does it line up in similar way with original cards that come with them, for example? Was about to comment that there seems to be more 2.0 in the wild as it was available via retail.

I'm fine with text entry like old phones and how the stock firmware works. Canned messages would be neat but regular entry would be great to have. Excited!!! Thank you!

Gnu-Bricoleur commented 3 weeks ago

Concerning the documentation, I agree it was necessary, here it is : https://github.com/meshtastic/meshtastic/pull/1283 I'm trusting your judgement on the naming, especially since Chatter V1.0 is now explicitly stated in the documentation

I'm not sure how mapping preregistered canned messages to some hotkeys would work but I feel like accessing preregistered messages is already fairly easy, the selection menu appears as soon as the UP or DOWN key is pressed. The rest of the keypad works as you would expect from a T9 keypad for free text input.

The layout is based on Chatter's keypad sticker so no surprises here, it's the same for V1 and V2. I included a picture in the documentation.

Let me know if you think of anything that should be included in this MR

Gnu-Bricoleur commented 3 weeks ago

(Writing the doc made me notice that for Chatter V2 owners, having the button register two different event each time it's pressed might cause issues, removing the USER_BUTTON mapping should prevent that)

eureekasigns commented 3 weeks ago

Decided to figure out how to build this and flashed a 2.0 Chatter. It does seem to work as expected, however with the up/down arrow keys used for canned messages, there does not seem to be a way to select a recipient for direct message. It's only possible to compose messages to the default channel.

Is there a way perhaps to allow shift key + arrow to select recipient? Or, perhaps after starting text entry, the arrow keys change function to select node? Very excited this works as it does. The only thing is selecting a node to direct message does not seem possible yet. Thank you.

Gnu-Bricoleur commented 3 weeks ago

I the destination field but couldn't manage to select it and change it. How do you add a node to your "contacts"? If you managed to do it, I think the best solution would be to only keep one key as the UP key to select the canned messages and one key as the RIGHT to select the destination as you might want to also change the destination of prerecorded canned messages and switching key functions with SHIFT is fiddly and not intuitive.

@eureekasigns , since you managed to build and flash your Chatter, can you just change line 92 of src/input/SerialKeyboard.cpp from e.inputEvent = meshtastic_ModuleConfig_CannedMessageConfig_InputEventChar_RIGHT; to e.inputEvent = meshtastic_ModuleConfig_CannedMessageConfig_InputEventChar_RIGHT; e.kbchar = 0xb7; and tell me if it works for you ?

eureekasigns commented 3 weeks ago

Agree the shift is not intuitive for such a function, and I like the idea of one for each separate function.

I will see if I can get some time in the next day or two to try the change in code, thank you!

eureekasigns commented 3 weeks ago

Had some time to test, and I think it's on the right track, but still unfortunately not able to select a node to send to. It still seems to just show "To: Broadcast@LongFast" on the top (or, whatever the default channel 0 is)

The key cycles between the nodes in the list, but when typing to send, it no longer selects a node as far as I can tell.

Gnu-Bricoleur commented 3 weeks ago

@eureekasigns It's the same for me ! Do you know if there's some kind of contact list you need to add the nodes to ? I couldn't find the option in the app or on the device. Are you sure sending canned messages to a specific destination is supported by the firmware right now ? As I said in the issue discussion, it's my first experimentations with Meshtastic so I'm not really aware of what's possible

tomhanax commented 3 weeks ago

Maybe I did not understand correctly, but you have to switch destSelect mode to be able to select channel / node:

case 0x09: // tab
                if (this->destSelect == CANNED_MESSAGE_DESTINATION_TYPE_CHANNEL) {
                    this->destSelect = CANNED_MESSAGE_DESTINATION_TYPE_NONE;
                } else if (this->destSelect == CANNED_MESSAGE_DESTINATION_TYPE_NODE) {
                    this->destSelect = CANNED_MESSAGE_DESTINATION_TYPE_CHANNEL;
                } else {
                    this->destSelect = CANNED_MESSAGE_DESTINATION_TYPE_NODE;
                }
                break;

I changed my code for T-Beam to up and down key to select mode:

case 0xb5: // up
                this->destSelect = CANNED_MESSAGE_DESTINATION_TYPE_NODE;
                break;
            case 0xb6: // down
                this->destSelect = CANNED_MESSAGE_DESTINATION_TYPE_NONE;
                break;
eureekasigns commented 3 weeks ago

@eureekasigns It's the same for me ! Do you know if there's some kind of contact list you need to add the nodes to ? I couldn't find the option in the app or on the device. Are you sure sending canned messages to a specific destination is supported by the firmware right now ? As I said in the issue discussion, it's my first experimentations with Meshtastic so I'm not really aware of what's possible

I am not sure it's fully supported, but I believe the T-Deck can select a node for private/direct message by scrolling the trackball left and right. That device is more capable obviously, but there are also struggles with the current firmware and there is a new UI being worked on for it.

I think it's supported but not well documented or established. I think it's supported to send to a node, though.

Perhaps the code in comment above is helpful in this idea?

If it's possible it would be very nice, but if it's not, it's still nice to be able to reply to default channel on device. It would also be nice to select channel to reply to, but that may be a separate struggle with limited buttons. On the T-Deck, that is also quirky with some keyboard shortcuts I think. Perhaps at least reply to node for private message is something that can be done on this. The way meshtastic handles the private message it should use a private channel to a node if it's been last seen on a private channel.

Gnu-Bricoleur commented 3 weeks ago

Yes, I've been looking at the code a bit more and it's definitely possible, I'll try to add that and complete the documentation as soon as I have a bit of free time.

Gnu-Bricoleur commented 2 weeks ago

It's now possible to select a destination by pressing TAB (SHIFT then BACKSPACE) to focus on the destination field and then cycle through possible contacts with RIGHT. It is not ideal but it feels like a good compromise to me given the limited number of buttons available on the Chatter. I'll update the doc MR to reflect those changes. @eureekasigns if you have the time, can you check that it now works as you expected ?

eureekasigns commented 2 weeks ago

It's now possible to select a destination by pressing TAB (SHIFT then BACKSPACE) to focus on the destination field and then cycle through possible contacts with RIGHT. It is not ideal but it feel like a good compromise to me given the limited number of buttons available on the Chatter. I'll update the doc MR to reflect those changes. @eureekasigns if you have the time, can you check that it now works as you expected ?

This works as expected! That's a good compromise on key combination. Easy enough to remember how to do, and I think even on the T-Deck it is less than ideal at the moment.

For this device, it is a very nice feature to have! I would say it all works as it should on the Chatter so far and I am hopeful that this PR can move forward when they take a look at moving it into a release! Thank you for the work on this. It is very cool to see!

eureekasigns commented 2 weeks ago

One note on messages to nodes. If you have a primary channel 0 that is not Longfast, such as a private primary channel, selecting a node to send to will only message the node on the default private channel. Delivery will not succeed unless the other node has the same private channel 0.

So, it seems channel selection is separate from node selection. I think the T-Deck may have a similar limitation on channel selection but do not know for sure as I don't have one

Something to be aware of. I'm not sure there's a great way to do channel selection on these devices, not only because of limited buttons but possibly general firmware limitations currently. Still very useful to be able to direct message a node!

tomhanax commented 2 weeks ago

Regarding to TAB key, which, as stated, is really not ideal - did you consider my suggestion about using up and down key? These keys are not used when composing a message an they are logical option - UP key moves from message line up to channel/node select line and DOWN moves down to message line. IMHO this will work as good as on T-Deck. Or am I missing something?

Gnu-Bricoleur commented 2 weeks ago

@eureekasigns , have you tried pressing twice on TAB ? Because of this :

if (this->destSelect == CANNED_MESSAGE_DESTINATION_TYPE_CHANNEL) {
     this->destSelect = CANNED_MESSAGE_DESTINATION_TYPE_NONE;
} else if (this->destSelect == CANNED_MESSAGE_DESTINATION_TYPE_NODE) {
     this->destSelect = CANNED_MESSAGE_DESTINATION_TYPE_CHANNEL;
 } else {
    this->destSelect = CANNED_MESSAGE_DESTINATION_TYPE_NODE;
}

It seems TAB cycle through the destinations types channels, nodes and none (?!?!) Then RIGHT cycle through the available contacts.

Gnu-Bricoleur commented 2 weeks ago

@tomhanax (I don't have experience with Meshtastic so I might be missing something) Default behavior of the UP key doesn't allow moving from message line up to channel/node select line at the moment, only TAB events works. I like having the UP key to select preregistered canned messages The DOWN key is now mapped to RIGHT so it allows to cycle through the UI screens when not typing a messages which is cool. When typing a message it lets you cycle recipients. I agree that using TAB (SHIFT + BACKSPACE) is not ideal but I don't want to edit CannedMessageModule.cpp as modifying it changes UI for everyone (definitely out of the scope of this MR) and adding a special case for Chatter seems unclean. I think it's better to be compliant with the current control scheme even if it's a bit inconvenient.

Best thing would be to change key function based on the current context (typing messages, selecting preregistered canned messages, looking at the telemetry screens, ..) but it is not supported by the firmware at the moment and again, out of scope of this MR

eureekasigns commented 2 weeks ago

@Gnu-Bricoleur thank you for the explanation on selection. I had missed that. This does work, though with how "tab" works, it's a little strange. I also need to compile the latest update with shift key change. That'll be helpful.

I think overall this is very good. It could never really be perfect with limited buttons, canned message support, multiple channels, and selecting a node.

What we have now is functional and I'm thankful!

Gnu-Bricoleur commented 1 week ago

I don't know what's the usual process to get merged but I'm happy with the state of this MR as is (and this one https://github.com/meshtastic/meshtastic/pull/1283). Let me know if there's anything (bug, architecture, code quality, ...) I can still do on this branch

(I have no idea how to fix the two CI fails however)

tomhanax commented 1 week ago

I switched to branch, and done some testing. Great work @Gnu-Bricoleur, much appreciated, we have 4 chatters so now they are definitely usable :)

However... now the bad news, I found some minor issues. I compare expected behavior with classic Nokia button cellphone, which, I believe, could be good example of how people expect things to work.

  1. I press letter button several (> 5) times. Instead of cycling three letters and number, there is fifth uppercase letter and then cursor moves. So for example pressing "2" quickly 6 times, it prints "a", "b", "c", "2", "A" and the next press moves the cursor. This is unexpected. It should cycle three letters and number forever, until you pause or press another button. So expected sequence is "a", "b", "c", "2", "a", "b".
  2. If I press "2" two times quickly and then press "3" quickly after that, I expect to enter "bd", instead, the "e" is typed. In other words, pressing another button in "letter selection" should skip to next character. The waiting should not be necessary.
  3. There is no indication about entering mode - uppercase, lowercase, number. I know you did not want to mess with core files, so I understand, but now it is not very user friendly.
  4. In uppercase mode backspace switches to channel/node selection. Unexpected... Why this way?
  5. In channel/node selection, only down key works, not up key.
  6. The "Smiley" button used work as USER button - screen switch, wake from sleep. Now it is dead (only works as mode selector). OK I found out that "Check" and "Cross" buttons work, so no problem here.

Hope this info is useful some way, I am not sure if you are willing to make some code corrections. Maybe I could help, but I am not good in C++, nor using github (only messaging like this), so I do not know exactly how to participate effectively. Anyway, If you have any idea how can I help, feel free to write!

eureekasigns commented 6 days ago

On point 5, this was the way to get channel/node selection working with the compromise of limited buttons. Believe it's in the doc linked here in the thread

No, it's not perfect but it does work. I've taken to starting a message then getting that set, then actually typing out a message.

The up was kept for canned message selection. Previously it used to be up or down.

Yeah it might be nicer to cycle letters a little more like an old phone but idk if that's a limit on core files. Kind of used to the way it works, it's usable anyway.

Imo good enough to include in an updated release but agree it could maybe be a little better. For what has been done it's basically about as feature complete as it might get for these devices.

Gnu-Bricoleur commented 5 days ago

@tomhanax , thanks for the thorough testing ! 😄 Point 1 & 2 seems to be bugs, I have an idea of what's causing that, I'll definitely try to tackle them whenever I get some free time, but not before the end of the week at best. Point 3 is as you surmised my unwillingness to mess with core functions of Meshtastic. Since there doesn't seem to be a readily available feature I can use to notify the user of the keyboard mode, I won't add it into this MR. I'm not sure how to do it properly, I don't have a good grasp on the codebase and I think it's cleaner if a MR equals a feature. Point 4 is due to the lack of available buttons. SHIFT + BACKSPACE is actually TAB. I could have mapped the TAB to something else but nothing felt really intuitive so I settled on the BACKSPACE key as it doesn't have too many functions already. Feel free to suggest a better key to map the TAB event if you have one in mind Point 5 is just the way it works, the UP/LEFT key is just UP and the DOWN/RIGHT key is just RIGHT. This is because in meshtastic you need both "hotizontal" and "vertical" movement and there is only two key available for the four directions but meshtastic fw automaitcally loops once you reached the end of a list so it's a way to have all the functions available at all time at the cost of a small inconvenience when you jump over the item you wanted to select and have to go back to the beginning of the list ...

Point 4 & 5 are discussed in the sister MR I have with the documentation for Chatter https://github.com/meshtastic/meshtastic/pull/1283 so hopefully, even if it is a bit unexpected people will still be able to figure it out quickly !

Gnu-Bricoleur commented 22 hours ago

@tomhanax I think I fixed points 1&2 ! Also the documentation for Chatter has been merged : https://meshtastic.org/docs/hardware/devices/chatter/ So even if I'm not "fixing" points 4&5 for the aforementioned reasons, it's now publicly documented. Thanks again for your feedback and I'm open to further input whenever you get time to test the latest fix !

eureekasigns commented 18 hours ago

Looks like it works and is definitely a good improvement here. Much appreciated, thank you!