keyboardio / Kaleidoscope-Bundle-Keyboardio

A Kaleidoscope distribution for the Keyboardio Model 01 and other keyboards.
Other
17 stars 24 forks source link

Fixed virtual builds #18

Closed noseglasses closed 4 years ago

noseglasses commented 5 years ago

This PR is one of a set of PRs that intents to fix Kaleidoscope virtual builds.

The following PRs need to be merged to fix virtual builds.


Outlook

This PR is just one (crucial) step of an ongoing work to add a simple and efficient virtual testbench for Kaleidoscope and its plugins. Virtual builds will be the integral part of it.

It is planned to add a simple testing API to enable virtual builds.

A sketch could then add optional code that is conditionally compiled only with virtual builds. This additional code will specify a test run. Code might look as follows (the actual testing code might end up more concise).


// In some_sketch.ino

...
#ifdef KALEIDOSCOPE_VIRTUAL_BUILD

namespace kaleidoscope {
namespace simulator {

void runSimulator(Simulator &simulator) {

   // Add an assertion that checks the next key report that is going to 
   // be issued
   //
   simulator.queueReportAssertion(
      reportKeysActive(Key_A, true /* exclusively */)
   );

   simulator.pressKey(Key_A);

   simulator.skipTime(500 /* ms */);

   simulator.releaseKey(Key_A);

   // Run one more cycle. During this cycle the assertion is applied
   // on the generated key report that follows from the above key release 
   // statement.
   //
   simulator.cycle();
}

} // namespace simulator
} // namespace kaleidoscope

#endif

Assertions of various types will be available, that enable to check what is reported, the state of LEDs, the EEPROM and what ever makes sense to be tested.

If one of the assertions fails, the testing firmware executable terminates and signals error state.

Compared to the existing approach that is already implemented by Kaleidoscope-Hardware-Virtual, which is to enter keys manually or supplying them via a script this approach is both simpler and more powerful. It is simpler because no external scripting is required and more powerful as many different state variables can be checked through assertions.

This can then also be used for debugging purposes to generate 100% reliable input. Complex plugins that do sophisticated things with keys and key-reports would benefit a lot from being tested automatically.

It might be a good idea to add test(...) functions to all examples and let them execute as part of a virtual kaleidocope build, e.g. by executing the test as an additional build step that is specified in plattform.txt.

By entering

ARCH=virtual make

a test sketch would be automatically build and executed.

This would also hold for the examples test-bench when build for the virtual core

# Build and run all example tests
ARCH=virtual make smoke-examples

Everything is going to live within Kaleidoscope's own build system, no scripting or additional tools will be involved.

The basic concepts of testing with assertions has already been implemented in Python as part of Leidokos-Python which unfortunately slightly fell into disrepair.

By bundling the testing API with the rest of the firmware bundle, it will be easy to maintain the testing system compatible with the firmware.

Edit: Virtual keyboard process faking a USB device

The Linux USB/IP project allows to mimic a USB device via software (see https://github.com/lcgamboa/USBIP-Virtual-USB-Device). I tested it and it seems to work also for USB keyboards. Using USB/IP it might be possible to simulate the serial communication with the keyboard. This approach could be useful to integrate virtual firmware builds with Chrysalis in a joined test bench.


Commit Description

This commit re-enables virtual builds with the Model01 keyboard.

With this change, virtual builds are further on triggered via setting ARCH=virtual. The board that firmware is meant to be build for is defined through the BOARD variable, just like with non-virtual builds.

Signed-off-by: Florian Fleissner florian.fleissner@inpartik.de

obra commented 5 years ago

First up, I am -absolutely- thrilled you're working on this. Thank you. I really like that virtual builds will be able to be done for any board.

The "Hardware Spec" being broken out into a separate header makes me a tiny bit uncomfortable, but I may be able to learn to live with it.

I'm a bit more uncomfortable with the change to add '-DKALEIDOSCOPE_HARDWARE_SPEC_H' for each and every board. It feels like we end up with...a lot of repetition.

It feels like there's some other alternate factoring where the board spec data gets pulled in or defined by the board hardware file.

It -might- make sense to have the hardware .h files and virtual versions thereof generated from, say json files that define the hardware attributes we're talking about.

I feel like I'm having a bit of trouble articulating what I'm talking about here. Am I making any sense?

noseglasses commented 5 years ago

The "Hardware Spec" being broken out into a separate header makes me a tiny bit uncomfortable, but I may be able to learn to live with it.

I'm a bit more uncomfortable with the change to add '-DKALEIDOSCOPE_HARDWARE_SPEC_H' for each and every board. It feels like we end up with...a lot of repetition.

I'm not sure if I understand what you mean. The avr-version of boards.txt does the same with -DKALEIDOSCOPE_HARDWARE_H for every board.

Would it help to avoid breaking out the hardware file and having just one hardware .h/.cpp file where half of it (namely the hardware class) is prevented from being compiled by #ifndef ARDUINO_VIRTUAL ... #endif? With such an approach we could symlink x86/boards.txt to avr/boards.txt.

It -might- make sense to have the hardware .h files and virtual versions thereof generated from, say json files that define the hardware attributes we're talking about.

That would mean to have some extra tool that does this mapping by parsing json and emitting c++ code. That's entirely possible but not a lot of fun and probably a lot of extra work unless relying on additional libraries for the parsing stuff. I'd rather keep everything simple.

obra commented 5 years ago

On Mon, Jun 3, 2019 at 2:19 AM noseglasses notifications@github.com wrote:

The "Hardware Spec" being broken out into a separate header makes me a tiny bit uncomfortable, but I may be able to learn to live with it.

I'm a bit more uncomfortable with the change to add '-DKALEIDOSCOPE_HARDWARE_SPEC_H' for each and every board. It feels like we end up with...a lot of repetition.

I'm not sure if I understand what you mean. The avr-version of boards.txt does the same with -DKALEIDOSCOPE_HARDWARE_H for every board.

Right. The concern was about requiring more than one custom macro for each board, if we can avoid it.

Would it help to avoid breaking out the hardware file and having just one hardware .h/.cpp file where half of it (namely the hardware class) is prevented from being compiled by #ifndef ARDUINO_VIRTUAL ... #endif? With such an approach we could symlink x86/boards.txt to avr/boards.txt.

Hrm. Having only one file would certainly be preferable. Rather than ifdeffing it out, could we instead create a kind of class that is a 'hardware definition', breaking out our hardware classes into a data class and an implementation class?

It -might- make sense to have the hardware .h files and virtual versions thereof generated from, say json files that define the hardware attributes we're talking about.

That would mean to have some extra tool that does this mapping by parsing json and emitting c++ code. That's entirely possible but not a lot of fun and probably a lot of extra work unless relying on additional libraries for the parsing stuff. I'd rather keep everything simple.

Understood

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope-Bundle-Keyboardio/pull/18?email_source=notifications&email_token=AAALC2CALL7OOENN5ERTHT3PYQFMZA5CNFSM4HSCOAC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWX3FLI#issuecomment-498053805, or mute the thread https://github.com/notifications/unsubscribe-auth/AAALC2BFAWW2GGCEAB6AIX3PYQFMZANCNFSM4HSCOACQ .

noseglasses commented 5 years ago

Hrm. Having only one file would certainly be preferable. Rather than ifdeffing it out, could we instead create a kind of class that is a 'hardware definition', breaking out our hardware classes into a data class and an implementation class?

I'm not exactly sure about the difference to what i did with the specification class. Could you elaborate on what you think belongs in either of the two classes that you envision.

Whatever approach we adopt, the key issue is to not let the compiler see the avr-related stuff in case of the virtual build but to still inform it about the important properties of the keyboards, so the virtual hardware can pick that up.

obra commented 5 years ago

Getting this right is something I feel will be really important in the future. I’m sorry I’ve done a poor job explaining myself. I’m in China visiting factories and have limited net and attention.

You’re right that starting from an (informal) spec is probably the right way to get to something good.

I’m going to try to explain again, but am writing from my phone in a car on a pretty bumpy road ;)

Right now, we would need to either ifdef the hardware specific bits OR pass in different headers to get a virtual build.

I -think- I’m thinking of a structure where a given keyboard device implementation

has-a description consisting of the logical key layout, led layout, crgb , etc (the etc needs to be filled in

And

Has-a hardware driver class.

For virtual builds, we could instantiate a different hardware driver class for a given logical description.

This isn’t quite the factoring I got to in atmegakeyboard but it’s where I was headed.

Jesse

Sent from my iPhone

On Jun 3, 2019, at 10:32 PM, noseglasses notifications@github.com wrote:

Hrm. Having only one file would certainly be preferable. Rather than ifdeffing it out, could we instead create a kind of class that is a 'hardware definition', breaking out our hardware classes into a data class and an implementation class?

I'm not exactly sure about the difference to what i did with the specification class. Could you elaborate on what you think belongs in either of the two classes that you envision.

Whatever approach we adopt, the key issue is to not let the compiler see the avr-related stuff in case of the virtual build but to still inform it about the important properties of the keyboards, so the virtual hardware can pick that up.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

noseglasses commented 5 years ago

Here's an update about my work on the virtual builds and a testing API.

The testing API is ready. i envision it to become part of the firmware bundle to let all plugins benefit from future simulator-based integration testing.

I've closed https://github.com/keyboardio/KeyboardioScanner/pull/17 as we can live with a redundant cRGB for Keyboardio keyboards.

The Model01 is still the only keyboard that has been given a specification header. IMHO, it is nonetheless OK to live with this inconsistency for the time being, until we decide how to best do the separation of specification/implementation of hardware classes. With the proposed changes we are already able to reanimated virtual builds at least for the Model01.

Yet to run all smoke-examples test with virtual builds and additional testing, we would need to supply specification headers for all other keyboards.

~The two tiny PRs https://github.com/keyboardio/Kaleidoscope/pull/652 and https://github.com/keyboardio/Kaleidoscope/pull/654 that have been separated from https://github.com/keyboardio/Kaleidoscope/pull/650 are still waiting to be merged.~

With the changes/PRs I've submitted so far, we could already add simulation-based integration testing with the Model01 for the stock firmware to travis. Even with the other keyboards not yet supported by virtual builds, this would already be a great leap ahead.

Having an actual test bench would make merging the pending PRs, especially the larger ones a great deal more safe.

I'd be happy to write some tests that check existing firmware feature once the necessary bits are merged.

noseglasses commented 5 years ago

Here's a link to a short video that demonstrates real time keyboard simulation now possible with the latest changes: https://ln.sync.com/dl/cc571b3a0/mqg7t4s3-uviea9hq-chaaas4y-6swczgvv

Keyboard key states are send via serial communication to the host that feeds it to the simulator within a simulation loop.

noseglasses commented 5 years ago

To allow for cRGB to be defined by KeyboardioScanner but also e.g. by the Model01's spec header, I submitted another PR https://github.com/keyboardio/KeyboardioScanner/pull/18.

noseglasses commented 5 years ago

I would like to postpone the transition to a joined virtual hardware repo and prioritize the availability of simulation based integration-testing.

With tests that actually run the firmware, it will be much safer to apply complex changes to the firmware and its overall architecture.

Once this PR has been merged, I could fix up some of the examples in the Kaleidoscope repo to demonstrate how my new simulation/testing API could be used for integration-testing.

If/once we agreed on what is tested how, the next step would be to teach travis to additionally build and run virtual smoke-examples.

noseglasses commented 5 years ago

i have removed the redundant and outdated code in Kaleidoscope-Hardware-Virtual. The virtual hardware now is a mere wrapper for the implementation of the physical hardware (the virtual hardware now uses KeyboardioHID and Kaleidoscope-HIDAdaptor-KeyboardioHID. Only the HID-class has been replaced by a stub implementation that collects HID reports.

In terms of keyboard HID reports (NKRO) this has been thoroughly tested but not yet validated. I am going to collect data of the real keyboard with all types of HID reports and will compare it with the simulator to see if all types of HID reports are handled equivalently on both sides.

noseglasses commented 5 years ago

This is an update about my work on testing. In the meantime I have designed and implemented Aglais an I/O protocol data format/library/toolset that greatly helps to record, compress and re-simulate recorded session from the original keyboard.

The exact timing of key-action and all sorts of HID reports is stored in a compressed text file format. Aglais files can right now be used to validate the simulator. Later on, when we are sure that the simulator can be trusted, Aglais could be used to generate complex test input in a very simple fashion.

Aglais comes with a small converter tool that enables converting back and forth between a human-readable version (for debugging) of files and a compressed one (storage optimal). The converter can also generate a quoted string version of Aglais-file content that can directly be copy/pasted into C++ Kaleidoscope test files.

See https://github.com/CapeLeidokos/Kaleidoscope-Simulator/blob/master/examples/aglais/tests.h for a simple example test. As this is an example, it uses the verbose version of Aglais files. To keep repo footprint of test files small, in real applications the compressed form of the files would be used, where each command keyboard is replaced by an integer command-ID.

See the following repos for more information.

obra commented 5 years ago

That’s really cool!

I think I’d prefer we always use the expanded, readable version of the format. In general, I’m a fan of readable textual protocols. The compressed version of the protocol doesn’t -really- buy us a ton of savings in the modern world. And there’s value in having a single representation of the format.

On Jun 16, 2019, at 11:54 AM, noseglasses notifications@github.com wrote:

This is an update about my work on testing. In the meantime I have designed and implemented Aglais an I/O protocol data format/library/toolset that greatly helps to record, compress and re-simulate recorded session from the original keyboard.

The exact timing of key-action and all sorts of HID reports is stored in a compressed text file format. Aglais files can right now be used to validate the simulator. Later on, when we are sure that the simulator can be trusted, Aglais could be used to generate complex test input in a very simple fashion.

Aglais comes with a small converter tool that enables converting back and forth between a human-readable version (for debugging) of files and a compressed one (storage optimal). The converter can also generate a quoted string version of Aglais-file content that can directly be copy/pasted into C++ Kaleidoscope test files.

See https://github.com/CapeLeidokos/Kaleidoscope-Simulator/blob/master/examples/aglais/tests.h for a simple example test. As this is an example, it uses the verbose version of Aglais files. To keep repo footprint of test files small, in real applications the compressed form of the files would be used, where each command keyboard is replaced by an integer command-ID.

See the following repos for more information.

Kaleidoscope-Simulator Kaleidoscope-Simulator-Recorder Aglais — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

noseglasses commented 5 years ago

The pending PR to Kaleidoscope now passes CI check. After the dependency PRs listed at the top of this page have been merged and before this can be merged, this PR must be rebased to reflect latest changes.

noseglasses commented 5 years ago

I rebased this PR after recent changes to submodules occurred in master. Also I had to add another dependency to a PR to Kaleidoscope-Hardware-Virtual.

Sorry for the frequent changes, PR and updates. But it is really hard to keep track when changes are required in a system with git submodules.

obra commented 5 years ago

You never need to apologize for frequent updates to volunteer work. (And indeed, we're trying to undo as many of the submodules as possible. Long term, are you cool moving the other parts of the simulation framework into the main core repo?) ᐧ

On Mon, Jun 17, 2019 at 4:04 AM noseglasses notifications@github.com wrote:

I rebased this PR after recent changes to submodules occurred in master. Also I had to add another dependency to a PR to Kaleidoscope-Hardware-Virtual.

Sorry for the frequent changes, PR and updates. But it is really hard to keep track when changes are required in a system with git submodules.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope-Bundle-Keyboardio/pull/18?email_source=notifications&email_token=AAALC2CXKOTTSAOTMMENKLTP25VVVA5CNFSM4HSCOAC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX22IFI#issuecomment-502637589, or mute the thread https://github.com/notifications/unsubscribe-auth/AAALC2FOZZ7DFXCN6LI7MDTP25VVVANCNFSM4HSCOACQ .

noseglasses commented 5 years ago

Long term, are you cool moving the other parts of the simulation framework into the main core repo?

Main core repo?

obra commented 5 years ago

Sorry, I meant moving "Kaleidoscope-Simulator" and friends into the Kaleidoscope repo. ᐧ

On Mon, Jun 17, 2019 at 2:46 PM noseglasses notifications@github.com wrote:

Long term, are you cool moving the other parts of the simulation framework into the main core repo?

Main core repo?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope-Bundle-Keyboardio/pull/18?email_source=notifications&email_token=AAALC2A4ZHK2N7PLD2AZYILP3AA5DA5CNFSM4HSCOAC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX4RMBI#issuecomment-502863365, or mute the thread https://github.com/notifications/unsubscribe-auth/AAALC2EIFL5NHQRQRDHDVYDP3AA5DANCNFSM4HSCOACQ .

noseglasses commented 5 years ago

The short answer is yes. Moving that stuff under Keyboardio's github organization is perfectly fine with me.

Now, the long answer. We need to find a good way to structure things. Kaleidoscope-Simulator uses Aglais which is entirely firmware implementation agnostic and designed as an independent piece of software (not necessarily an Arduino lib). It is shared by Kaleidoscope-Simulator and Kaleidoscope-Simulator-Recorder which both currently include it as a git submodule in their Arduino src-dir. i would prefer to keep Aglais a standalone repo, independent from Kaleidoscope (which doesn't mean that we cannot move it to github-Keyboardio).

Kaleidoscope-Simulator is also in most parts pretty independent from Kaleidoscope. It's rather a simuator driver than an actual simulator as the actual simulator/virtual hardware lives in the Arduino core and Kaleidoscope-Hardware-Virtual. That's why just today i started planning to introduce an interface layer that separates all the Kaleidoscope-specific stuff and to make it really independent. I was already thinking about a species whose name I could use for this generic keyboard simulator driver library.

Although I really like Kaleidoscope, I tend to think and design things in a way that they are as flexible as possible to potentially be used by other projects.

What do you think?

obra commented 5 years ago

That all sounds perfectly reasonable. It -may- be the case that we end up deciding it makes sense for Kaleidoscope to snapshot releases of Aglais, rather than include it as a submodule, but that's not really here or there.

Mostly, I wanted to make sure that we weren't working toward making Kaleidoscope's simulation framework be something that you were absolutely opposed to us merging into the core repos/org.

On Mon, Jun 17, 2019 at 3:19 PM noseglasses notifications@github.com wrote:

The short answer is yes. Moving that stuff under Keyboardio's github organization is perfectly fine with me.

Now, the long answer. We need to find a good way to structure things. Kaleidoscope-Simulator uses Aglais which is entirely firmware implementation agnostic and designed as an independent piece of software (not necessarily an Arduino lib). It is shared by Kaleidoscope-Simulator and Kaleidoscope-Simulator-Recorder which both currently include it as a git submodule in their Arduino src-dir. i would prefer to keep Aglais a standalone repo, independent from Kaleidoscope (which doesn't mean that we cannot move it to github-Keyboardio).

Kaleidoscope-Simulator is also in most parts pretty independent from Kaleidoscope. It's rather a simuator driver than an actual simulator as the actual simulator/virtual hardware lives in the Arduino core and Kaleidoscope-Hardware-Virtual. That's why just today i started planning to introduce an interface layer that separates all the Kaleidoscope-specific stuff and to make it really independent. I was already thinking about a species whose name I could use for this generic keyboard simulator driver library.

Although I really like Kaleidoscope, I tend to think and design things in a way that they are as flexible as possible to potentially be used by other projects.

What do you think?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope-Bundle-Keyboardio/pull/18?email_source=notifications&email_token=AAALC2G4M2GAAS2UE67GX7DP3AEYJA5CNFSM4HSCOAC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX4TRIQ#issuecomment-502872226, or mute the thread https://github.com/notifications/unsubscribe-auth/AAALC2ABCCE34ZHAZ23F5WLP3AEYJANCNFSM4HSCOACQ .

noseglasses commented 5 years ago

Mostly, I wanted to make sure that we weren't working toward making Kaleidoscope's simulation framework be something that you were absolutely opposed to us merging into the core repos/org.

I'd be happy for it to become part of the butterfly-zoo.

noseglasses commented 5 years ago

The simulator just learned to convert keyboard, mouse and absolute mouse reports to X11 events. Now it is possible to entirely simulate the keyboard in realtime. This works as follows:

1) Key action on a physical Kaleidoscope-driven keyboard (not necessarily the simulated one) 2) Key position is passed to the simulator running on the host (Kaleidoscope-Simulator-Control, serial) 3) Simulator considers the virtual key-event 4) Virtual Kaleidoscope generates HID reports 5) Reports are converted to x11events (e.g. keycodes) using XTest 6) X11 maps displays keycodes and moves the pointer

The fun thing about this is that it enables arbitrarily complex plugins due to the absence of resource restrictions when the firmware is running on the host :-)

Now there is just one thing missing which is to pass the LED colors back to the physical keyboard...

noseglasses commented 5 years ago

@obra, why does the absolute mouse report consider only one wheel while the relative mouse considers two in the HID report data? Is that some restriction that comes from the USB HID spec?

obra commented 5 years ago

Because support for the second wheel came later as a contribution. The PR text says "vertical" but the code was for "horizontal"

Note that changes to this stuff are -very- fiddly and require a lot of OS testing on every platform we support.

commit 78caf1ee3048dda7b10ae8f44011a428a9242a06 Author: SjB steve@sagacity.ca Date: Tue Nov 7 00:05:59 2017 -0500

Support for Vertical mouse wheel movement.

Extended the HID Descriptor to send a Consumer Device page
that support a vertical mouse will message

On Wed, Jun 19, 2019 at 5:11 AM noseglasses notifications@github.com wrote:

@obra https://github.com/obra, why does the absolute mouse report consider only one wheel while the relative mouse considers two in the HID report data? Is that some restriction that comes from the USB HID spec?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope-Bundle-Keyboardio/pull/18?email_source=notifications&email_token=AAALC2F3VJ5OG7WVEBHNJ6TP3IO57A5CNFSM4HSCOAC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYBVDSQ#issuecomment-503534026, or mute the thread https://github.com/notifications/unsubscribe-auth/AAALC2FX3UJZ2KGAHJTK4F3P3IO57ANCNFSM4HSCOACQ .

noseglasses commented 5 years ago

Thanks for clarifying. I am not up to changing this. One can always rely on the non-absolute mouse report for using both wheels.

noseglasses commented 5 years ago

i have now validated all types of HID reports (keyboard, boot keyboard, mouse, absolute mouse). All of them are now correctly passed through Kaleidoscope-HIDAdaptor-KeyboardioHID and KeyboardioHID on to the simulator.

Arbitrary typing sessions can be recorded on the real keyboard and replayed in the simulator for integration testing purposes. This also allowed me to validate the simulator with respect to the generation and timing of the above mentioned report types.

For all of the supported report types X11 events are correctly generated (if enabled). This allows for real-time simulation of all features of the keyboard.

It is important to mention that @gedankenexperimenter's recent work on eliminating all "asynchronous" calls to millis() was a key change. Without it, i.e. if millis() is called during cycles, the behavior of the physical keyboard is not reproducible as the simulator cannot know the exact timing of the actual hardware. This leads me to the conclusion that we should emphasize on the importance of not using millis() directly or even find a way to make it unavailable to user code.

noseglasses commented 5 years ago

I have recently separated the Kaleidoscope-independent part from Kaleidoscope-Simulator. It was moved to a new library called Papilio that contains the simulation infrastructure.

Kaleidoscope-Simulator does not come with submodules anymore but depends on the two independent projects Papilio and Aglais. Both are Arduino libraries.

@obra, an incorporation of Kaleidoscope-Simulator in Kaleidoscope would be possible if any .h and .cpp files that are now part of Kaleidoscope-Simulator would be equipped with #ifdef KALEIDOSCOPE_VIRTUAL_BUILD clauses to prevent them being compiled for the device. Aglais and Papilio could either become submodules of the bundle or be snapshots of releases of the two libraries.

obra commented 5 years ago

Awesome! ᐧ

On Wed, Jun 26, 2019 at 6:33 AM noseglasses notifications@github.com wrote:

I have recently separated the Kaleidoscope-independent part from Kaleidoscope-Simulator. It was moved to a new library called Papilio https://github.com/CapeLeidokos/Papilio that contains the simulation infrastructure.

Kaleidoscope-Simulator does not come with submodules anymore but depends on the two independent projects Papilio and Aglais. Both are Arduino libraries.

@obra https://github.com/obra, an incorporation of Kaleidoscope-Simulator in Kaleidoscope would be possible if any .h and .cpp files that are now part of Kaleidoscope-Simulator would be equipped with #ifdef KALEIDOSCOPE_VIRTUAL_BUILD clauses to prevent them being compiled for the device. Aglais and Papilio could either become submodules of the bundle or be snapshots of releases of the two libraries.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope-Bundle-Keyboardio/pull/18?email_source=notifications&email_token=AAALC2DCB7DDL36ZXMZORODP4NVZFA5CNFSM4HSCOAC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYTRAAI#issuecomment-505876481, or mute the thread https://github.com/notifications/unsubscribe-auth/AAALC2BXNILAGU6WVSW4Y33P4NVZFANCNFSM4HSCOACQ .

obra commented 5 years ago

I'm going to try to start merging this work into the core. I'm still not entirely happy with the way we're splitting apart the logical and physical description of a keyboard, but I'd like to get as much of the code merge in as possible so that the diffs we're discussing are manageable. That -may- mean that I merge in some changes that I don't intend to keep in the interest of getting things moved forward.

noseglasses commented 4 years ago

Just force-pushed an updated versions of the changes necessary to reenable virtual builds after Kaleidoscope's big device API redesign.

noseglasses commented 4 years ago

It looks like travis fails because the Kaleidoscope submodule version that is part of the PR is not published and can thus not be found.

I suggest that I eventually push an update once https://github.com/keyboardio/Kaleidoscope/pull/650 has been merged.

noseglasses commented 4 years ago

Travis fails for this PR.

@obra, I ran make travis-test-all on my Ubuntu box. it looks like most tests pass and only cpplint complains. Here's a gist of the output.

noseglasses commented 4 years ago

cpplint generates a lot of false positives.

We might consider using clang-tidy. I just tried it with a Kaleidoscope virtual build and it works nicely. It even has a pretty decent auto-fix mode that deals with a lot of problematic cases and could be used as an additional make target like make astyle.

There would, however, remain two problems to be solved.

1) How to integrate clang-tidy in the build system (only possible for virtual builds, anyway) 2) Select a suitable set of linter tests

obra commented 4 years ago

Yup. I'm working through this now.

travis-test-all runs the more strict versions of the cpplint tests and indeed, there are a bunch of false positives. It's not the target we actually use to test. It's the one we historically use as a development aid.

That said, some of what it's complaining about seems reasonable enough to fix.

But since it's not actually a regression here, I'm merging.

obra commented 4 years ago

Merged. Closing.

obra commented 4 years ago

cpplint -does- generate fewer false positives when we pick a saner set of policies. I'd be very happy to have clang-tidy as an option too. ᐧ

On Thu, Dec 5, 2019 at 10:14 AM noseglasses notifications@github.com wrote:

cpplint generates a lot of false positives.

We might consider using clang-tidy. I just tried it with a Kaleidoscope virtual build and it works nicely. It even has a pretty decent auto-fix mode that deals with a lot of problematic cases and could be used as an additional make target like make astyle.

There would, however, remain two problems to be solved.

  1. How to integrate clang-tidy in the build system (only possible for virtual builds, anyway)
  2. Select a suitable set of linter tests

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/keyboardio/Kaleidoscope-Bundle-Keyboardio/pull/18?email_source=notifications&email_token=AAALC2CKPWR4P3G2E3CQK6DQXFAIBA5CNFSM4HSCOAC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGBT2HI#issuecomment-562248989, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALC2AKPSLB27POVCL5FQDQXFAIBANCNFSM4HSCOACQ .