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

Add both Ergodox EZ and Infinity Ergodox as sub-projects of Ergodox #503

Merged
merged 20 commits into from Jul 30, 2016

Conversation

Projects
None yet
6 participants
@fredizzimo
Copy link
Collaborator

fredizzimo commented Jul 10, 2016

The two keyboards can now share all the layers, and every keymap compiles in for both keyboards. This was actually surprisingly easy to do, I just had to replace the LED control functions with dummy implementations, and do some very minor fixes(config.h and include paths) to some keymaps.

The next step is to add a compatibility Visualizer, so that the LED control functions actually does something useful on the Infinity. The idea is that all keyboards would at least get this, but it would be also possible to override it with a full fledged visualizer, which would probably display the layer names on the LCD and so on.

This also fixes #501.

I have tested this on the Infinity. But I don't own an EZ, so that would need to be tested before merging. It would also be good if this was carefully reviewed.

@jackhumbert

This comment has been minimized.

Copy link
Member

jackhumbert commented Jul 10, 2016

I'm definitely a fan of this, but I'd like to leave this decision completely up to @ezuk, while applying some gentle encouragement :) it looks like the EZ is the default build project, so new EZ users shouldn't have any problem configuring things. I think having the ergodox/readme.md EZ-oriented with an Infinity footnote would be appropriate, but that's something else I'd like to leave up to @ezuk.

@fredizzimo

This comment has been minimized.

Copy link
Collaborator

fredizzimo commented Jul 10, 2016

Yes, the documentation needs some re-structuring. Currently that too is moved into the EZ folder, while the Ergodox and Infinity folders lack documentation completely.

I just read the Ez documentation, and it seems like most of the stuff there would apply to both keyboards, only the process of flashing is slightly different, the rest is the same. I also noticed a couple of references to the ergodox_ez folder, which should be changed to ergodox

I will fix the issues in the documentation now and move the readme from the ez subfolder back to the ergodox folder, as the file could be the same for both of them. I'll make a separate commit that unifies the documentation for EZ and Infinity, so that @ezuk, can separately decide if he accept that revision, or if it needs further fixing.

Regarding the migration. For users who have got their keymap included in the firmware, I don't think there will be any problems, GIT should easily resolve the move from ergodox_ez to ergodox. For people who haven't sent pull request, an maintain their keymaps in their own fork, there might be slight problems. I'm not sure if Git would notice the move when pulling a new version.

At least they would also manually have to change #include "ergodox_ez.h" to #include "ergodox.h". I don't think any other changes were needed to get all the keyboards to compile for the Ez. The Infinity compilation might additionally need some config.h and Makefile changes, especially if they are poorly done.

I can do some testing in Git to see how a fork would behave.

@fredizzimo

This comment has been minimized.

Copy link
Collaborator

fredizzimo commented Jul 10, 2016

I have updated the readme file, are there more things that need to be done?

1. Download and install the [Teensy Loader](https://www.pjrc.com/teensy/loader.html). Some Linux distributions already provide a binary (may be called `teensy-loader-cli`), so you may prefer to use this.
2. Find a firmware file you like. You can find a few if these in the keymaps subdirectory right here. The file you need ends with .hex, and you can look at its .c counterpart (or its PNG image) to see what you'll be getting. You can also use the [Massdrop configurator](https://keyboard-configurator.massdrop.com/ext/ergodox) to create a firmware Hex file you like.
2. Find a firmware file you like. You can find a few of these in the keymaps subdirectory right here. The file you need ends with .hex, and you can look at its .c counterpart (or its PNG image) to see what you'll be getting. You can also use the [Massdrop configurator](https://keyboard-configurator.massdrop.com/ext/ergodox) to create a firmware Hex file you like.

This comment has been minimized.

@fredizzimo

fredizzimo Jul 10, 2016

Collaborator

I believe this section needs to be rewritten, since the hex files are no longer stored in the repository.

@ezuk

This comment has been minimized.

Copy link
Member

ezuk commented Jul 11, 2016

I believe I mentioned this a couple of times before: It's quite important for me to keep the EZ and the Infinity clearly separated. This is both for commercial reasons and for support reasons. I'd like to have separate folders and separate readmes, even if the underlying code supports both.

@fredizzimo

This comment has been minimized.

Copy link
Collaborator

fredizzimo commented Jul 11, 2016

@ezuk

I'm sorry, but I thought we agreed on this here

  keyboard
    <layout_name>  # EG, ergodox
      keymaps
      <architecture_1>
      <architecture_2>
      ...
      <architecture_N>

Which is how this pull request structures it

  keyboards
    ergodox
      keymaps
      ez
      infinity

And I thought that was enough for separation. Regarding this comment

I.e, it needs to be immediately clear wheter it's the EZ or the Infinity someone is using. This is crucial for support and troubleshooting purposes. These are two distinct products (commercially) and the firmware should clearly reflect this. Otherwise confusion ensues :)

I thought it's more a question about how people report bugs, than how the code is structured. And I haven't seen, or remember any more comments regarding that.

But I will remove the common code, and readme from the Ergodox folder, and duplicate it to the Ez and Infinity folder, if you prefer that instead.

The software architect inside me cringes of the thought of doing that though, but you are the one with commercial interest, so it's completely up to you. One my core pillars as a software architect is to never, ever duplicate code. There's two major reasons for that

  1. Duplicated code is much harder to maintain
  2. It's very easy to have bugs, by fixing things in one place but forgetting about the other.

In this case, since the keymaps are completely compatible, it means that when changes are made, we would very likely break some keymap on one of the architectures. On the other hand with the structure of the pull request, that is very unlikely to happen, as can be seen by how little changes I had to do to the keymaps themselves,

If you go one step further, and want to separate the keyboards completely, so that not even the keymaps are shared, then we are dealing with even more duplication. A lot of the keymaps are useful for many people, independent of the keyboard they are using, and when maintaining those we have to make sure to apply the same changes to both locations.

That said, for the keyboard part the amount of duplicated code is not huge, and if I'm going to maintain the Infinity part in the future too, I can manage that.

For the keyboard layouts, I would probably make my own layout, and one default layout. And let the community duplicate and maintain the other layouts if they want to. That way it wouldn't be a huge headache for me either.

But let me be clear, I'm not personally offended in any way, no matter which decision you make, I completely understand your reasons. And I also understand if you don't want to share the keymaps at all, in order to not give some competitive advantage to the competitor. However, remember that the firmware is open source, and someone could make a fork with the exact same changes as this pull request, and then you would have almost the same situation (I will not do that, so far both you and @jackhumbert have been very friendly and helpful, and I'm very happy to make some compromises because of that)

So which structure exactly do you want?

@sethbc

This comment has been minimized.

Copy link
Contributor

sethbc commented Jul 12, 2016

I tend to agree with the reasoning that @fredizzimo laid out above. Obviously defer to @erez as to where this ultimately shakes out but it seems to me like there should be a way to address @erez's concerns while preserving as much common code as possible.

My unsolicited 2 cents on this is that there is value in having common keymaps. I'm waiting on my EZ, but I've got a couple Ergodox Infinity that i'm looking to get set up on QMK and would love to be able to use a single keymap for the two different boards and just swap subprojects when building.

That being said, I'm curious what the directory structure looks like if the Infinity/EZ ends up releasing additional revisions (that generally look the same but with some tweaks, see e.g. the planck). Are we looking at 'infinity1' / 'infinity2' or 'ez1' / 'ez2' ? And how would code-sharing going to work between those versions?

@ezuk

This comment has been minimized.

Copy link
Member

ezuk commented Jul 12, 2016

What can I say, you guys. You make a ton of a sense. I'm a dev too, and I hate code duplication as much as the next guy.

From a technical standpoint this makes sense. Let's do the right thing here -- go with @fredizzimo's approach, which is clearly sane.

I only hope that this indeed doesn't increase support burden in the future and I don't find myself in the awkward position of having to offer commercial-grade support to people who are not actually ErgoDox EZ customers. If in the future it doesn't work out, I'm sure we'll figure something out.

For now, let's do the sane thing. Thanks for insisting and reasoning so clearly, I appreciate it very much!

@@ -0,0 +1,110 @@
#ifndef KEYBOARDS_INFINITY_ERGODOX_INFINITY_ERGODOX_H_

This comment has been minimized.

@sethbc

sethbc Jul 12, 2016

Contributor

should this be updated to KEYBOARDS_ERGODOX_INFINITY_INFINITY_H_?

Don't think it's referenced elsewhere, but just for consistency.

This comment has been minimized.

@fredizzimo

fredizzimo Jul 12, 2016

Collaborator

Yes, I will change that a bit later today. The KEYBOARDS_ERGODOX_INFINITY_PART is not exactly according to the documentation either, but Eclipse, which I'm using adds the folder names so I have left them.

This mixup was probably because I moved stuff around.

@fredizzimo

This comment has been minimized.

Copy link
Collaborator

fredizzimo commented Jul 12, 2016

Thanks @ezuk,

First of all, I'm sorry for perhaps overacting yesterday, or at least sounding like that.

It's now clear that before we merge this, we should think more about the support issues. I think at least the readme should be updated, so that it clearly states that the Ez is officially supported, and the infinity is community supported. I can do that, according to your suggestion, or you can update it shortly after the pull request is merged.

And of course this is not only related to the Ergodox, as there are also some other keyboards not commercially supported by QMK.

And as I said earlier, we should also not be in too hurry to accept it, as there's always a small chance that it breaks something, which is something I don't want. So if you guys could merge it locally in your own repository first, and run with it for a while, it would be great.

I also think that it would make sense to adapt some convention for issues, like adding a tag indicating which keyboard and perhaps keymap the issue is being reported on. Like [Ergodox Default] Description of the problem. This would of course not be enforced, but we can document this, and change titles of existing issues, and due to the example most of the users would probably start to report things this way. For issues not related to any keyboard, mostly if it's clearly discussing code only we could add the [QMK] tag.

This tags should be based on what's initially reported, and not where the final issue is.

We could also perhaps adapt some folder naming convention, like "infinity_community", if it's a community supported keyboard.

@Lknechtli

This comment has been minimized.

Copy link

Lknechtli commented Jul 13, 2016

I've noticed that while starting up, the infinity can sometimes get into a weird state where the keybinds don't make any sense. This happens almost 100% of the time when I boot the computer with it connected, but can rarely happen when the computer is already on. I've also noticed a similar problem on the input.club firmware (where the modifiers do not work), but in this firmware it seems like the key matrix isn't being initialized correctly or something. Might be an issue that is more easily exposed during USB driver initialization?

It's not a serious issue, though, since all you need to do is restart the keyboard to fix it.

@fredizzimo

This comment has been minimized.

Copy link
Collaborator

fredizzimo commented Jul 13, 2016

@Lknechtli

I know you have the LED stuff stubbed out, and the visualizer isn't supported ATM, but is there a way to turn down / off the right LCD display on the infinity? The glare can get a bit annoying.

The visualizer will be supported pretty soon, within a few days. Once that's done you should easily be able to turn the LCD off. I will also make sure that with LCD_BACKLIGHT_ENABLE = no, it will be completely dark. A little bit after that I will implement brightness control by key mappings.

Any thoughts on how the lcds will be supported, if the keymaps need to be compatible with the Ez and Infinity?

First of all, there will be a default visualizer that will emulate the EZ led commands, by showing different colors on the screen.

To customize the visualization, you will be able to make an Infinity specific visualizer in this location ergodox/infinity/keymaps/keymap_name/visualizer.c. This Visualizer will be passive in a sense, that it will just listen to keyboard events, so it won't need any deep integration with the keymaps, and therefore will be completely decoupled from the Ergodox EZ. For some very special cases, the keymap could expose some global variables that the Visualizer can use.

For actual control I recommend that you look at the default visualizer in my TMK port https://github.com/fredizzimo/infinity_ergodox/blob/master/visualizer_user.c, and also the demo that I wrote for testing the LEDs https://github.com/fredizzimo/tmk_visualizer/blob/led/led_test.c. Both the LCD and LEDs are exposed as uGFX displays, which allows both individual pixel access, and high level drawing routines like text display.

The basic idea will be the same, but I'm still debating with myself, and now that this is now part of QMK, probably with others as well, if I should move the code to C++. That would be much safer, as I can make sure that you get compile errors if you do things wrong, and also generally would need much less user code. Technically there's no problem with C++ the compilers we are using supports it, and the Visualizer stuff needs more powerful processors anyway, where it's not critical to save absolutely every byte of memory.

6. Compile your firmware by running `make keymap=keymap_name`. For example, `make keymap=german`. This will result in a hex file, which will be called `ergodox_ez_keymap_name.hex`, e.g. `ergodox_ez_german.hex`. For **Infinity ErgoDox** you need to add `subproject=infinity` to the make command.
7. **ErgoDox EZ** - Flash this hex file using the [Teensy loader](https://www.pjrc.com/teensy/loader.html) as described in step 4 in the "Easy Way" above. If you prefer you can automatically flash the hex file after successful build by running `make teensy keymap=keymap_name`.

**Infinity ErgoDox** - Flash the firmware by running `make dfu-util keymap=keymap_name subproject=infinity`

This comment has been minimized.

@Skrymir

Skrymir Jul 15, 2016

Contributor

Is there a way to specify a device ID or serial number here?

One of the built in devices on my MBP reports in dfu. So there are two devices when the keyboard is connected

This comment has been minimized.

@fredizzimo

fredizzimo Jul 15, 2016

Collaborator

Currently it's not possible to do so from the make command directly. However you could run this command make bin keymap=keymap_name subproject=infinity which will generate a bin file ".build/ergodox_infinity_keymap_name.bin."

You can then flash this file manually using `dfu-util --serial .build/ergodox_infinity_keymap_name.bin.

I can add support for this to the make file at some point.

Edit: The default target should also be changed to bin for the Infinity, as the hex files are useless. That bin file could then be uploaded the same way as the hex files for the other keyboards. To support the basic way of flashing.

@fredizzimo fredizzimo force-pushed the fredizzimo:ergodox_subproject branch from 296e513 to 82d8747 Jul 28, 2016

@fredizzimo

This comment has been minimized.

Copy link
Collaborator

fredizzimo commented Jul 28, 2016

I rebased this on top of master to fix the conflicts.

@fredizzimo fredizzimo force-pushed the fredizzimo:ergodox_subproject branch from 82d8747 to eb28875 Jul 29, 2016

@fredizzimo

This comment has been minimized.

Copy link
Collaborator

fredizzimo commented Jul 29, 2016

I did one more rebase to fix the new conflicts.

fredizzimo added some commits Jul 7, 2016

Initial structure for Ergodox as subprojects
Only the EZ default keymaps compiles at the moment though.
Most ergodox keymaps compiles on Infinity
There are linker errors due to missing led funcitonality though
Add empty led control functions to Infinity Ergodox
So that most keymaps compiles and links

@fredizzimo fredizzimo force-pushed the fredizzimo:ergodox_subproject branch from eb28875 to d776e57 Jul 29, 2016

fredizzimo added some commits Jul 9, 2016

Speed up ChibiOS keymap compilation
By sharing the external library object files between the whole
keyboard, instead of re-compiling them for each keymap.
Define weak matrix user function for Infinity Ergodox
So that a few keyboards, which don't use them, links properly.

@fredizzimo fredizzimo force-pushed the fredizzimo:ergodox_subproject branch from d776e57 to 566c795 Jul 29, 2016

@fredizzimo

This comment has been minimized.

Copy link
Collaborator

fredizzimo commented Jul 29, 2016

And yet another rebase. This breaks very easily when keyboards are updated, especially when new ones are added, or when new files are added. Since I need to move them to the correct folder during the rebase. In this case the heat map of the new algernon update.

Since it's getting a bit tiresome to constantly fix this pull request. @ezuk and @jackhumbert. What's the plan with this? I can think of three options.

  1. Merge this now
  2. Tell me when it's ready to merge, so I can rebase it again to fix the conflicts
  3. Tell me that you won't accept it, and let's close this.

Since I'm not sure how much this has been reviewed yet, here's a short overview of what needed to be done to the actual keymaps, in order to enable this.

For almost all keyboards, this has been the process to move it to the new structure.

  1. Move it from keyboards/ergodox_ez/keymaps to keyboards/ergodox/keymaps.
  2. Change the include from ´ergodox_ez.hto ´ergodox.h
  3. Additionally for the algernon keymap I had to change _delay_ms to wait.ms, and include "wait.h" (there's actually a new delay in the latest version, so that failed travis, but show now be fixed.
  4. And the townk_os had the config file done in an incompatible way, so that was changed.
@jackhumbert

This comment has been minimized.

Copy link
Member

jackhumbert commented Jul 29, 2016

Yeah, I'm with you there. I'm fine with merging this in now, but I think Erez wanted to test things out more(?) - @ezuk is there a date you would like to commit to in the future as a deadline for testing/evaluating, or would you like to go ahead and merge?

@Lknechtli

This comment has been minimized.

Copy link

Lknechtli commented Jul 29, 2016

FWIW, I've been using this for 2 weeks on both the Ez and Infinity without any real issues other than small stuff that explicitly isn't supported at the moment on the Infinity. Obviously limited to my personal keymap.

@Skrymir

This comment has been minimized.

Copy link
Contributor

Skrymir commented Jul 29, 2016

I've had the same experience as @Lknechtli running both an Ez and Infinity. Also been holding off on keymap updates because I know its a pain with this branch.

@ezuk

This comment has been minimized.

Copy link
Member

ezuk commented Jul 30, 2016

alright -- let's take the plunge then. I'm okay with merging. If anything happens, @jackhumbert and @fredizzimo got my back ;)

@jackhumbert jackhumbert merged commit 0639836 into qmk:master Jul 30, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jackhumbert

This comment has been minimized.

Copy link
Member

jackhumbert commented Jul 30, 2016

Merged :D

ryaninvents pushed a commit to ryaninvents/qmk_firmware that referenced this pull request Aug 12, 2016

Merge pull request qmk#503 from fredizzimo/ergodox_subproject
Add both Ergodox EZ and Infinity Ergodox as sub-projects of Ergodox

@fredizzimo fredizzimo deleted the fredizzimo:ergodox_subproject branch Jun 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment