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

Cleanup the C++ ReflexGame and add an enclaved based keyboard input reactor #88

Merged
merged 4 commits into from
Apr 22, 2024

Conversation

cmnrd
Copy link
Contributor

@cmnrd cmnrd commented Oct 5, 2023

This revises the C++ implementation of the ReflexGame. I never really liked that the game logic is split between two reactors in the original implementation. This change revises the overall architecture. There is now one reactor implementing the game logic, one responsible for the random delay, and one responsible for the keyboard input.

Inspired by yesterday's discussion, this PR also adds another version of the ReflexGame which uses an enclave instead of an additional thread to handle the blocking calls to getchar(). This is probably also interesting for @erlingrj.

@cmnrd cmnrd requested review from edwardalee and lhstrh October 5, 2023 12:01
@erlingrj
Copy link
Contributor

erlingrj commented Oct 5, 2023

Cool, thanks for the ping. I will look at implementing something similar with C, just need to get some bugs under control first :)

Copy link
Contributor

@edwardalee edwardalee left a comment

Choose a reason for hiding this comment

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

Very nice! I like the enclave version.

One big question: the playground examples in Cpp are laid out differently than in C. You have playground-lingua-franca/examples/Cpp/ReflexGame/src/ReflexGame.lf whereas in C, we have playground-lingua-franca/examples/C/src/ReflexGame/ReflexGame.lf.

These have two different views of what a "project" is. In C, it is the collection of examples. In Cpp, each example is its own project.

The Cpp approach does not work well with Epoch. In Epoch, you either have to separately create a project for each example (nobody is going to do that), or else you get different layouts depending on whether you compile within Epoch vs. using lfc.

Can we change the Cpp layout to match the C layout (and Python)?

Also, I get a number of compiler warnings when compiling these examples:

% lfc-dev ReflexGame/src/ReflexGame.lf
lfc: info: Generating code for: file:/Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/src/ReflexGame.lf
lfc: info: Generation mode: STANDALONE
lfc: info: Generating sources into: /Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/src-gen/ReflexGame
Path: /Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/src-gen/ReflexGame /Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/src-gen/ReflexGame
--- Current working directory: /Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/build
--- Executing command: cmake --version
cmake version 3.27.6

CMake suite maintained and supported by Kitware (kitware.com/cmake).
--- Current working directory: /Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/build
--- Executing command: cmake -DCMAKE_BUILD_TYPE=Release -DREACTOR_CPP_VALIDATE=ON -DREACTOR_CPP_PRINT_STATISTICS=OFF -DREACTOR_CPP_TRACE=OFF -DREACTOR_CPP_LOG_LEVEL=3 -DLF_SRC_PKG_PATH=/Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame -DCMAKE_INSTALL_PREFIX=/Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame -DCMAKE_INSTALL_BINDIR=bin /Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/src-gen
-- The CXX compiler identification is AppleClang 15.0.0.15000040
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE  
-- Configuring done (12.2s)
-- Generating done (0.1s)
-- Build files have been written to: /Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/build
--- Current working directory: /Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/build
--- Executing command: cmake --build . --target ReflexGame --parallel 12 --config Release
[  5%] Building CXX object reactor-cpp-default/lib/CMakeFiles/reactor-cpp-default.dir/environment.cc.o
[ 10%] Building CXX object reactor-cpp-default/lib/CMakeFiles/reactor-cpp-default.dir/assert.cc.o
[ 21%] Building CXX object reactor-cpp-default/lib/CMakeFiles/reactor-cpp-default.dir/port.cc.o
[ 26%] Building CXX object reactor-cpp-default/lib/CMakeFiles/reactor-cpp-default.dir/logical_time.cc.o
[ 26%] Building CXX object reactor-cpp-default/lib/CMakeFiles/reactor-cpp-default.dir/action.cc.o
[ 36%] Building CXX object reactor-cpp-default/lib/CMakeFiles/reactor-cpp-default.dir/time.cc.o
[ 42%] Building CXX object reactor-cpp-default/lib/CMakeFiles/reactor-cpp-default.dir/scheduler.cc.o
[ 42%] Building CXX object reactor-cpp-default/lib/CMakeFiles/reactor-cpp-default.dir/multiport.cc.o
[ 57%] Building CXX object reactor-cpp-default/lib/CMakeFiles/reactor-cpp-default.dir/reaction.cc.o
[ 57%] Building CXX object reactor-cpp-default/lib/CMakeFiles/reactor-cpp-default.dir/reactor_element.cc.o
[ 57%] Building CXX object reactor-cpp-default/lib/CMakeFiles/reactor-cpp-default.dir/reactor.cc.o
[ 63%] Linking CXX shared library libreactor-cpp-default.dylib
[ 63%] Built target reactor-cpp-default
[ 73%] Building CXX object ReflexGame/CMakeFiles/ReflexGame.dir/ReflexGame/RandomDelay.cc.o
[ 73%] Building CXX object ReflexGame/CMakeFiles/ReflexGame.dir/ReflexGame/GameLogic.cc.o
[ 94%] Building CXX object ReflexGame/CMakeFiles/ReflexGame.dir/ReflexGame/ReflexGame.cc.o
[ 94%] Building CXX object ReflexGame/CMakeFiles/ReflexGame.dir/ReflexGame/_lf_preamble.cc.o
[ 94%] Building CXX object ReflexGame/CMakeFiles/ReflexGame.dir/main.cc.o
[ 94%] Building CXX object ReflexGame/CMakeFiles/ReflexGame.dir/ReflexGame/KeyboardInput.cc.o
In file included from /Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/src-gen/ReflexGame/ReflexGame/KeyboardInput.cc:7:
/Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/src-gen/ReflexGame/ReflexGame/KeyboardInput.hh:31:22: warning: private field '__lf_parameters' is not used [-Wunused-private-field]
    const Parameters __lf_parameters;
                     ^
In file included from /Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/src-gen/ReflexGame/ReflexGame/RandomDelay.cc:7:
/Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/src-gen/ReflexGame/ReflexGame/RandomDelay.hh:60:48: warning: private field 'min_delay' is not used [-Wunused-private-field]
  const typename Parameters::__lf_min_delay_t& min_delay = __lf_inner.min_delay;
                                               ^
/Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/src-gen/ReflexGame/ReflexGame/RandomDelay.hh:61:48: warning: private field 'max_delay' is not used [-Wunused-private-field]
  const typename Parameters::__lf_max_delay_t& max_delay = __lf_inner.max_delay;
                                               ^
In file included from /Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/src-gen/ReflexGame/ReflexGame/GameLogic.cc:7:
/Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/src-gen/ReflexGame/ReflexGame/GameLogic.hh:31:22: warning: private field '__lf_parameters' is not used [-Wunused-private-field]
    const Parameters __lf_parameters;
                     ^
2 warnings generated.
1 warning generated.
In file included from /Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/src-gen/ReflexGame/ReflexGame/ReflexGame.cc:7:
/Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/src-gen/ReflexGame/ReflexGame/ReflexGame.hh:33:22: warning: private field '__lf_parameters' is not used [-Wunused-private-field]
    const Parameters __lf_parameters;
                     ^
1 warning generated.
1 warning generated.
[100%] Linking CXX executable ReflexGame
[100%] Built target ReflexGame
--- Current working directory: /Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/build
--- Executing command: cmake --build . --target install --parallel 12 --config Release
Install the project...
-- Install configuration: "Release"
SUCCESS (compiling generated C++ code)
Generated source code is in /Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/src-gen/ReflexGame
Compiled binary is in /Users/edwardlee/git/playground-lingua-franca/examples/Cpp/ReflexGame/bin
lfc: warning: private field 'max_delay' is not used [RandomDelay.hh:61:48]
 --> ReflexGame/src/ReflexGame.lf:1:1 - 

lfc: warning: private field 'min_delay' is not used [RandomDelay.hh:60:48]
 --> ReflexGame/src/ReflexGame.lf:1:1 - 

lfc: info: Code generation finished.

@@ -2,104 +2,97 @@
* This example illustrates the use of logical and physical actions, asynchronous external inputs,
* the use of startup and shutdown reactions, and the use of actions with values.
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably should acknowledge the original source of the example:

 * The example is fashioned after an Esterel implementation given by Berry and Gonthier in "The
 * ESTEREL synchronous programming language: design, semantics, implementation," Science of Computer
 * Programming, 19(2) pp. 87-152, Nov. 1992, DOI: 10.1016/0167-6423(92)90005-V.

examples/Cpp/ReflexGame/src/ReflexGame.lf Outdated Show resolved Hide resolved
examples/Cpp/ReflexGame/src/ReflexGame.lf Show resolved Hide resolved
@lhstrh
Copy link
Member

lhstrh commented Oct 10, 2023

these have two different views of what a "project" is. In C, it is the collection of examples. In Cpp, each example is its own project.

The Cpp approach does not work well with Epoch. In Epoch, you either have to separately create a project for each example (nobody is going to do that), or else you get different layouts depending on whether you compile within Epoch vs. using lfc.

We have been advocating for the latter approach, because this is how languages with a package manager work (and we will soon have a package manager, too); you group all files that constitute an application together in a package, and different applications would reside in different packages. See #34.

@edwardalee
Copy link
Contributor

these have two different views of what a "project" is. In C, it is the collection of examples. In Cpp, each example is its own project.
The Cpp approach does not work well with Epoch. In Epoch, you either have to separately create a project for each example (nobody is going to do that), or else you get different layouts depending on whether you compile within Epoch vs. using lfc.

We have been advocating for the latter approach, because this is how languages with a package manager work (and we will soon have a package manager, too); you group all files that constitute an application together in a package, and different applications would reside in different packages. See #34.

Can we make it part of the effort then to fix how Epoch handles packages? Currently, it will create src-gen and bin in the package root, which is not what lfc does.

Or should we view this as another step towards burying Epoch?

@lhstrh
Copy link
Member

lhstrh commented Oct 11, 2023

Can we make it part of the effort then to fix how Epoch handles packages? Currently, it will create src-gen and bin in the package root, which is not what lfc does.

That is what lfc does, but Eclipse superimposes a notion of a project that does not match our definition of a package (in part because it creates a ghost filesystem that it does things in, IIRC). I think that we've tried to align these things in the past but concluded that we couldn't. It would actually make things a lot simpler if Epoch would behave the same as our CLI tools and VS Code extension, but I don't think Eclipse is designed this way. Perhaps @a-sr can comment on this.

Or should we view this as another step towards burying Epoch?

There is a continuous effort by myself and other to keep Epoch afloat. Every CI run checks that Epoch keeps working, and we publish a nightly release and made it installable through the installation script. Nobody is trying to bury epoch. But I don't think we should make decisions about how to lay out source trees, configure packages, and design our package manager based on how Eclipse works.

@cmnrd
Copy link
Contributor Author

cmnrd commented Oct 11, 2023

The Cpp approach does not work well with Epoch. In Epoch, you either have to separately create a project for each example (nobody is going to do that), or else you get different layouts depending on whether you compile within Epoch vs. using lfc.

It is bad that Epoch cannot handle this well... But I think the current directory layout is really how it should be.

Can we make it part of the effort then to fix how Epoch handles packages? Currently, it will create src-gen and bin in the package root, which is not what lfc does.

Yes, I think we should consider this a bug in Epoch. @lhstrh to clarify Edwards statement: In Epoch you can add the Cpp directory (or any directory up the hierarchy) as a project. The src-gen and other directories are then created in the project root, not the package root. This must be a problem in FileConfig.

@cmnrd
Copy link
Contributor Author

cmnrd commented Oct 11, 2023

Also, I get a number of compiler warnings when compiling these examples:

Thanks for reporting those. These warnings are not related to the program here and consider "problems" in the generated code. I don't see them, but this is because usually I don't use clang.

@edwardalee
Copy link
Contributor

Can we make it part of the effort then to fix how Epoch handles packages? Currently, it will create src-gen and bin in the package root, which is not what lfc does.

That is what lfc does, but Eclipse superimposes a notion of a project that does not match our definition of a package (in part because it creates a ghost filesystem that it does things in, IIRC). I think that we've tried to align these things in the past but concluded that we couldn't. It would actually make things a lot simpler if Epoch would behave the same as our CLI tools and VS Code extension, but I don't think Eclipse is designed this way. Perhaps @a-sr can comment on this.

Or should we view this as another step towards burying Epoch?

There is a continuous effort by myself and other to keep Epoch afloat. Every CI run checks that Epoch keeps working, and we publish a nightly release and made it installable through the installation script. Nobody is trying to bury epoch. But I don't think we should make decisions about how to lay out source trees, configure packages, and design our package manager based on how Eclipse works.

It occurs to me that there is no reason that the examples in the playground have to be laid out as one example per project. A project can consist of multiple programs regardless of how the package manager works, no? I think the examples should be one project per target language.

@lhstrh
Copy link
Member

lhstrh commented Oct 11, 2023

The Cpp approach does not work well with Epoch. In Epoch, you either have to separately create a project for each example (nobody is going to do that), or else you get different layouts depending on whether you compile within Epoch vs. using lfc.

It is bad that Epoch cannot handle this well... But I think the current directory layout is really how it should be.

Can we make it part of the effort then to fix how Epoch handles packages? Currently, it will create src-gen and bin in the package root, which is not what lfc does.

Yes, I think we should consider this a bug in Epoch. @lhstrh to clarify Edwards statement: In Epoch you can add the Cpp directory (or any directory up the hierarchy) as a project. The src-gen and other directories are then created in the project root, not the package root. This must be a problem in FileConfig.

I don't think it is a bug -- it was intentionally designed that way. It's hard to find generated files or binaries if they end up being buried somewhere deeply inside of an open project. The only way to avoid this, is to open a new project for each package, which @edwardalee stated no one would ever do. So, what is the solution then?

For the record, I'm fine unifying the behavior and placing generated files close to the project sources, inside of the boundaries of the package, but I recall that doing so was perceived as problematic by Epoch users in the past...

@lhstrh
Copy link
Member

lhstrh commented Oct 11, 2023

It occurs to me that there is no reason that the examples in the playground have to be laid out as one example per project. A project can consist of multiple programs regardless of how the package manager works, no? I think the examples should be one project per target language.

While it is possible to have multiple programs in a single package, the reason for putting them in the same package should be that they are related in some significant way. Programs could share code or configurations, for instance. This is not the case for most examples.

It becomes more apparent why lumping unrelated programs into the same package is probably not the best idea once we start moving the target properties from the LF files into the package configuration...

@cmnrd
Copy link
Contributor Author

cmnrd commented Oct 11, 2023

It occurs to me that there is no reason that the examples in the playground have to be laid out as one example per project. A project can consist of multiple programs regardless of how the package manager works, no? I think the examples should be one project per target language.

I think we are conflating the package concept with the concept of projects in Eclipse. An LF package is a directory that contains an src directory (and, once we have the package manager, a package configuration). An Eclipse project is a directory that is imported as a project in Eclipse.

I agree that there is no reason for which a project shouldn't be able to contain multiple programs. In fact, a project could contain multiple packages and each package may also contain multiple executables. This is exactly how the directory layout works right now. We can see the entire playground as a project, just a language subset, or just a single package. We and any user can freely choose what to consider a project and which scope to open in an editor. It is not the directory layout that is problematic, but Epoch's current requirements and assumptions about projects.

@a-sr
Copy link
Contributor

a-sr commented Oct 11, 2023

Can we make it part of the effort then to fix how Epoch handles packages? Currently, it will create src-gen and bin in the package root, which is not what lfc does.

That is what lfc does, but Eclipse superimposes a notion of a project that does not match our definition of a package (in part because it creates a ghost filesystem that it does things in, IIRC). I think that we've tried to align these things in the past but concluded that we couldn't. It would actually make things a lot simpler if Epoch would behave the same as our CLI tools and VS Code extension, but I don't think Eclipse is designed this way. Perhaps @a-sr can comment on this.

I think most IDEs have the concept of a project that defines the entry point into the file systems displayed in the IDE, Eclipse just has multiple to not impose a common parent for your components.
The layout with src, bin, and src-gen on the root seems closely motivated by Java bundles/jars (or Eclipse plugins), and how Xtext generated code integrates into this structure. While this is common practice, I don't think we have to adhere to it strictly.
I am fine with having a project at a higher level and then behave the same way as lfc, with generated code for separate packages/programs next to their src folder. At least in my mind, a project does not have to be equivalent to a single artifact or compilation unit.

If the Xtext framework actually enforces that you do it this way, with src/src-gen on the root/project level, I would propose stop using this specific API.
I think you are using the Generator infrastructure of Xtext because it is the recommended way. But again I don't think this is necessary. The main purpose of the Generator is to weave into the Eclipse building mechanism, which didn't work for us in the first place, and is the reason why we introduced the compile button.
I guess you use the context provided by the Xtext Generator quite often in the LF code generator, so it would be a mayor refactoring. However, from my perspective a normal Java class for the LF code generator that only takes an io.File or LF Model instance and works from there, would also work. The compile button code would afterwords make sure that whatever the code generation changes in the file systems will be synchronized with the project shown in Epoch. Yet, I guess this is only necessary if the location of src-gen cannot be changed to our needs.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

LGTM!

@lhstrh lhstrh merged commit 8b6541c into main Apr 22, 2024
4 of 5 checks passed
@lhstrh lhstrh deleted the cpp-reflex branch April 22, 2024 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants