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

CharShift: Configuration plugin #1069

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ namespace plugin {
// CharShift class variables
CharShift::KeyPair const * CharShift::progmem_keypairs_{nullptr};
uint8_t CharShift::num_keypairs_{0};
CharShift::GetNumKeyPairsFunction CharShift::numKeyPairs_ =
CharShift::numProgmemKeyPairs;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like this is unnecessary, but I'm not sure I'm right. Couldn't the same num_keypairs_ variable be used, regardless of where the data is stored? As long as we're reading values from either PROGMEM or EEPROM, but not both (perhaps by using an index offset), there's no need to use three bytes to store a separate count, plus the function pointer, right? Or are you thinking the user might want to switch back and forth between the two without power-cycling the keyboard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking that a user might want to use both at the same time, store some pairs in PROGMEM, some in EEPROM. Not sure if that makes sense for CharShift - it does for Macros and TapDance, where I copied the basic architecture from.

CharShift::ReadKeyPairFunction CharShift::readKeyPair_ =
CharShift::readKeyPairFromProgmem;

bool CharShift::reverse_shift_state_{false};

Expand Down Expand Up @@ -117,24 +121,12 @@ bool CharShift::isCharShiftKey(Key key) {

CharShift::KeyPair CharShift::decodeCharShiftKey(Key key) {
uint8_t i = key.getRaw() - ranges::CS_FIRST;
if (i < numKeyPairs()) {
return readKeyPair(i);
if (i < numKeyPairs_()) {
return readKeyPair_(i);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have we been using trailing underscores for private class function names as well as variables?

[My brain doesn't like the underscore immediately followed by the parens for some reason, but I'd much rather overhaul all the code to follow a consistent style than satisfy trivial preferences like that.]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I don't like it either. And no, we've been using trail-less private function names in the past. In this case, it's there because I first added the function pointers, then removed the original functions. It's safe and fine to rename them.

}
return {Key_NoKey, Key_NoKey};
}

// This should be overridden if the KeyPairs array is stored in EEPROM
__attribute__((weak))
uint8_t CharShift::numKeyPairs() {
return numProgmemKeyPairs();
}

// This should be overridden if the KeyPairs array is stored in EEPROM
__attribute__((weak))
CharShift::KeyPair CharShift::readKeyPair(uint8_t n) {
return readKeyPairFromProgmem(n);
}

uint8_t CharShift::numProgmemKeyPairs() {
return num_keypairs_;
}
Expand Down
51 changes: 39 additions & 12 deletions plugins/Kaleidoscope-CharShift/src/kaleidoscope/plugin/CharShift.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,27 @@ class CharShift : public Plugin {
num_keypairs_ = _num_keypairs;
}

typedef uint8_t (*GetNumKeyPairsFunction)();
typedef KeyPair (*ReadKeyPairFunction)(uint8_t n);

static void setNumKeyPairsFunction(GetNumKeyPairsFunction f) {
numKeyPairs_ = f;
}
static void setReadKeyPairFunction(ReadKeyPairFunction f) {
readKeyPair_ = f;
}

friend class CharShiftConfig;

private:
// A pointer to an array of `KeyPair` objects in PROGMEM
static KeyPair const * progmem_keypairs_;
// The size of the PROGMEM array of `KeyPair` objects
static uint8_t num_keypairs_;

static GetNumKeyPairsFunction numKeyPairs_;
static ReadKeyPairFunction readKeyPair_;

// If a `shift` key needs to be suppressed in `beforeReportingState()`
static bool reverse_shift_state_;

Expand All @@ -91,28 +106,40 @@ class CharShift : public Plugin {
/// Look up the `KeyPair` specified by the given keymap entry
static KeyPair decodeCharShiftKey(Key key);

/// Get the total number of KeyPairs defined
///
/// This function can be overridden in order to store the `KeyPair` array in
/// EEPROM instead of PROGMEM.
static uint8_t numKeyPairs();

/// Get the `KeyPair` at the specified index from the defined `KeyPair` array
///
/// This function can be overridden in order to store the `KeyPair` array in
/// EEPROM instead of PROGMEM.
static KeyPair readKeyPair(uint8_t n);

// Default for `keypairsCount()`: size of the PROGMEM array
static uint8_t numProgmemKeyPairs();
// Default for `readKeypair(i)`: fetch the value from PROGMEM
static KeyPair readKeyPairFromProgmem(uint8_t n);
};

class CharShiftConfig: public Plugin {
public:
CharShiftConfig() {}

EventHandlerResult onNameQuery();
EventHandlerResult onFocusEvent(const char *command);

static void setup(uint8_t dynamic_offset, uint8_t max_pairs);

private:
static uint8_t max_pairs_;
static uint16_t storage_base_;
static uint8_t dynamic_offset_;

static uint8_t numEEPROMPairs() {
return max_pairs_;
}
static CharShift::KeyPair readKeyPairFromEEPROM(uint8_t n);

static uint8_t numPairs();
static CharShift::KeyPair readKeyPair(uint8_t n);
};

} // namespace plugin
} // namespace kaleidoscope

extern kaleidoscope::plugin::CharShift CharShift;
extern kaleidoscope::plugin::CharShiftConfig CharShiftConfig;

/// Define an array of `KeyPair` objects in PROGMEM
///
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
/* -*- mode: c++ -*-
* Kaleidoscope-CharShift -- Independently assign shifted and unshifted symbols
* Copyright (C) 2021 Keyboard.io, Inc
*
* This program is free software: you can redistribute it and/or modify it under
* the terms of the GNU General Public License as published by the Free Software
* Foundation, version 3.
*
* This program is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
* FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
* details.
*
* You should have received a copy of the GNU General Public License along with
* this program. If not, see <http://www.gnu.org/licenses/>.
*/

#include "kaleidoscope/plugin/CharShift.h"

#include <Kaleidoscope-FocusSerial.h>
#include <Kaleidoscope-EEPROM-Settings.h>

#include "kaleidoscope/Runtime.h"

namespace kaleidoscope {
namespace plugin {

// =============================================================================
// CharShiftConfig class variables

uint8_t CharShiftConfig::max_pairs_;
uint8_t CharShiftConfig::dynamic_offset_;
uint16_t CharShiftConfig::storage_base_;

// =============================================================================
// Event handlers

EventHandlerResult CharShiftConfig::onNameQuery() {
return ::Focus.sendName(F("CharShiftConfig"));
}

EventHandlerResult CharShiftConfig::onFocusEvent(const char *command) {
if (::Focus.handleHelp(command, PSTR("charshift.map")))
return EventHandlerResult::OK;

if (strcmp_P(command, PSTR("charshift.map")) != 0)
return EventHandlerResult::OK;

if (::Focus.isEOL()) {
// We dump key by key, rather than pairs, because the end result is the
// same, and dumping one by one is less code.
for (uint16_t i = 0; i < max_pairs_ * 2; i += 2) {
Key k;

Runtime.storage().get(storage_base_ + i, k);
::Focus.send(k);
}
} else {
uint16_t pos = 0;

// We read one key at a time, rather than a keypair, to better handle
// partials and failure, and to make the code simpler.
while (!::Focus.isEOL()) {
Key k;

::Focus.read(k);
Runtime.storage().put(storage_base_ + pos, k);
pos += 2;
}
Runtime.storage().commit();
}

return EventHandlerResult::EVENT_CONSUMED;
}

// =============================================================================
// Support functions

void CharShiftConfig::setup(uint8_t dynamic_offset, uint8_t max_pairs) {
dynamic_offset_ = dynamic_offset;
max_pairs_ = max_pairs;

storage_base_ = ::EEPROMSettings.requestSlice(max_pairs * 4);
::CharShift.setNumKeyPairsFunction(numPairs);
::CharShift.setReadKeyPairFunction(readKeyPair);
}

CharShift::KeyPair CharShiftConfig::readKeyPairFromEEPROM(uint8_t n) {
uint16_t pos = storage_base_ + n * 4; // 4: Size of a keypair.
uint16_t raw_lower = Runtime.storage().read(pos);
uint16_t raw_upper = Runtime.storage().read(pos + 2);

return CharShift::KeyPair(Key(raw_lower), Key(raw_upper));
}

uint8_t CharShiftConfig::numPairs() {
return CharShift::numProgmemKeyPairs() + numEEPROMPairs();
}

CharShift::KeyPair CharShiftConfig::readKeyPair(uint8_t n) {
if (n < dynamic_offset_) {
return CharShift::readKeyPairFromProgmem(n);
}
return readKeyPairFromEEPROM(n - dynamic_offset_);
}

} // namespace plugin
} // namespace kaleidoscope

kaleidoscope::plugin::CharShiftConfig CharShiftConfig;