search

esrille / new-keyboard

The Esrille New Keyboard
https://www.esrille.com/keyboard/
47 stars 22 forks source link

Store the prefixshift setting in the initial NVRAM data #14

Closed jeandeaual closed 3 years ago

jeandeaual commented 3 years ago

This doesn't matter much since the default is currently 0 (PREFIXSHIFT_OFF), but can be useful if updating the default settings like I do in https://github.com/jeandeaual/new-keyboard/commit/82ea7b0ff12e7d89cd72fd78a6cb7ef5f372af5e.

ShikiOkasaka commented 3 years ago

Thanks for your pull request. We will add CONTRIBUTING.md to this project shortly, so you can review it.

There are two things that need to be fixed:

1) At this point, we maintain NISSE firmware configurations for both PIC18F4550 and PIC18F47J53. If NVRAM_DATA for PIC18F47J53 is to be modified, the following file must also be modified:

firmware/third_party/mla_v2013_12_20/bsp/esrille_new_keyboard/nvram.h

2) We define *_MAX by digits in this project; the order of the defined names are changed occasionally.

jeandeaual commented 3 years ago

At this point, we maintain NISSE firmware configurations for both PIC18F4550 and PIC18F47J53. If NVRAM_DATA for PIC18F47J53 is to be modified, the following file must also be modified:

firmware/third_party/mla_v2013_12_20/bsp/esrille_new_keyboard/nvram.h

Thanks for the information.

We define *_MAX by digits in this project; the order of the defined names are changed occasionally.

I see. I wasn't sure since KANA_MAX is defined as KANA_MTYPE (unless set in the project configuration) and PAD_SENSE_MAX is defined as PAD_SENSE_4.

Thanks for your pull request. We will add CONTRIBUTING.md to this project shortly, so you can review it.

Thanks for creating this document. I wasn't sure if pull requests were welcome or not. I fixed the issues you mentioned, but since this PR is not based off an issue, feel free to close it. I'll make sure to follow the instructions in CONTRIBUTING.md next time.

ShikiOkasaka commented 3 years ago

__EEPROM_DATA() takes 8 arguments. So NVRAM_DATA() would need to be expanded like,

__EEPROM_DATA(a, b, c, d, e, f, g, h); __EEPROM_DATA(i, 0, 0, 0, 0, 0, 0, 0)

I'm going to close this PR for now; please review the CONTRIBUTING.md file.

jeandeaual commented 3 years ago

__EEPROM_DATA() takes 8 arguments. So NVRAM_DATA() would need to be expanded like,

__EEPROM_DATA(a, b, c, d, e, f, g, h); __EEPROM_DATA(i, 0, 0, 0, 0, 0, 0, 0)

I see. I'd need both hardware to test if I were to make a PR. Thanks for the review.