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

Cannot use after delay with user-defined types #1958

Closed
edwardalee opened this issue Aug 16, 2023 · 7 comments · Fixed by #1959
Closed

Cannot use after delay with user-defined types #1958

edwardalee opened this issue Aug 16, 2023 · 7 comments · Fixed by #1959
Assignees
Labels
bug Something isn't working

Comments

@edwardalee
Copy link
Collaborator

If you use a typedef to define type and import a file with the type defined in a file-scoped preamble, the type is not made available to the generated delay reactors. This makes it very difficult (impossible?) to use after delays with user-defined types unless everything is one file.

To reproduce, put the following into a file GenDelay.lf:

target C

preamble {=
  #ifndef GEN_DELAY
  #define GEN_DELAY
  typedef int message_t;
  #endif
=}

reactor Source {
  output out: message_t
  reaction(startup) -> out {=
    lf_set(out, 42);
  =}
}

reactor Sink {
  input in: message_t
  reaction(in) {=
    lf_print("Received %d at time %lld", in->value, lf_time_logical_elapsed());
  =}
}

Then put this into another file, e.g. GenDelayTest.lf:

target C
import Source, Sink from "GenDelay.lf"

main reactor {
  source = new Source()
  sink = new Sink()
  source.out -> sink.in after 10 ms
}

The file fails to compile with this error:

llij/test/C/src-gen/GenDelayTest/./include/GenDelayTest/_lf_GenDelay_49429363.h:39:5: error: unknown type name 'message_t'
    message_t value;
    ^
/Users/eal/lingua-franca-intellij/test/C/src-gen/GenDelayTest/./include/GenDelayTest/_lf_GenDelay_49429363.h:55:5: error: unknown type name 'message_t'
    message_t value;
    ^
In file included from /Users/eal/lingua-franca-intellij/test/C/src-gen/GenDelayTest/__lf_gendelay_49429363.c:3:
/Users/eal/lingua-franca-intellij/test/C/src-gen/GenDelayTest/./__lf_gendelay_49429363.h:13:5: error: unknown type name 'message_t'
    message_t value;
@edwardalee edwardalee added the bug Something isn't working label Aug 16, 2023
@petervdonovan
Copy link
Collaborator

Our options are:

  1. Ensure that the delay reactor has access to the declarations that are used by one of the child reactors that is being connected. The type of the port should be in the intersection of the types used by all the child reactors involved, so it shouldn't matter which child we pick. Furthermore, it should only access the declarations of one reactor (i.e., it cannot have declarations of both the child and the parent) because if it did then there could be name collisions.
  2. Declare that this is the desired behavior. In this case, users should define a common header file with their typedefs, and include that header file in files that explicitly use those typedefs and in files that "rely on those typedefs through their use of the interfaces of reactors which expose the typedefs." In this case, if you connect a reactor using an after delay, you are implicitly accessing the type that is exposed by the port of the reactor, and in order to access that type, you need to #include the header file.

The advantage of option 1 is that it closes this issue and eliminates one pitfall that users might run into. It is a simple solution that is very likely to fully fix this problem.

The advantage of option 2 is that it places responsibility on the user to understand how the #includes in the C target work. The reason why this is an advantage is that it might be inevitable anyway; after all, it isn't possible to write code in C or C++ without understanding all the subtle details of #includes. If we decide that it is our responsibility to clean up those subtle details while building our system directly on top of them, then we may have a long road ahead of us.

@edwardalee
Copy link
Collaborator Author

I prefer option 1. Option 2 will also require specifying the included header file in the target files directive, adding complication. Also, rather the a separate header file, a more elegant way to share declarations across reactor definitions in separate files is inheritance.

Note that we often get name collisions anyway. I find that usually need to guard everything in a file-level preamble with #ifndef, as I did in this case:

preamble {=
  #ifndef NRF_FD
  #define NRF_FD
  enum message_type {
    heartbeat,
    pingNRP,
    pingNRP_response,
    request_new_NRP,
    new_NRP
  };
  typedef struct message_t {
    enum message_type type;
    int source;
    int destination;
  } message_t;
  #endif // NRF_FD
=}

If we remove the #ifndef, we get name collisions.

@petervdonovan
Copy link
Collaborator

petervdonovan commented Aug 17, 2023

Option 2 will also require specifying the included header file in the target files directive, adding complication. Also, rather the a separate header file, a more elegant way to share declarations across reactor definitions in separate files is inheritance.

The files directive may need to be used in many nontrivial programs. Furthermore, in my experience it is common for some set of type definitions to be used in several places throughout a program. In this case, it is not obvious to me that having every reactor in a program inherit from the reactor that carries the typedefs is the most elegant way to share declarations.

If we remove the #ifndef, we get name collisions.

This sounds like a mistake on my part because we should be automatically wrapping those in an include guard named TOP_LEVEL_PREAMBLE followed by a hash.


Anyway, it will be no problem for me to implement option 1. I will do that today.

@cmnrd
Copy link
Collaborator

cmnrd commented Aug 18, 2023

I am lacking some understanding about the implementation of C preambles here, but it seems like the C++ implementation could serve as a good template. After all preambles in conjunction with modular code generation have been working and tested for some years now.

Note that we often get name collisions anyway. I find that usually need to guard everything in a file-level preamble with #ifndef, as I did in this case:

I find this strange. Why not guard the preamble automatically in the code generator? In the C++ target, every public preamble becomes a header file and the include guards are added automatically. There is also no hashing needed, as they can be named uniquely within one program. Making a preamble visible, then means that the corresponding header gets included. This can be done completely automatically.

@petervdonovan
Copy link
Collaborator

Why not guard the preamble automatically in the code generator?

As I said, that is what we do. It would be helpful to have a minimal reproduction that shows why our mechanism does not work.

There is also no hashing needed, as they can be named uniquely within one program.

I'm not sure what this means. The purpose of hashing is to provide a unique name to include guard. Also, hashing is just an implementation detail that the user does not usually need to worry about, so although I'm sure there are other ways to do this, I am not sure what would be wrong with the current approach.

1 similar comment
@petervdonovan
Copy link
Collaborator

Why not guard the preamble automatically in the code generator?

As I said, that is what we do. It would be helpful to have a minimal reproduction that shows why our mechanism does not work.

There is also no hashing needed, as they can be named uniquely within one program.

I'm not sure what this means. The purpose of hashing is to provide a unique name to include guard. Also, hashing is just an implementation detail that the user does not usually need to worry about, so although I'm sure there are other ways to do this, I am not sure what would be wrong with the current approach.

@edwardalee
Copy link
Collaborator Author

An example that fails to compile without manually guarding the preamble can be found in the leader-election branch of the playground-lingua-franca repo. The file is examples/C/src/leader-election/NRP_FD.lf. If you remove the guard in the preamble, then lfc reports this:

In file included from /Users/eal/git/playground-lingua-franca/examples/C/src-gen/leader-election/NRP_FD_Partitioning/_nrp_fd_partitioning_main.c:3:
In file included from /Users/eal/git/playground-lingua-franca/examples/C/src-gen/leader-election/NRP_FD_Partitioning/./_nrp_fd_partitioning_main.h:4:
/Users/eal/git/playground-lingua-franca/examples/C/src-gen/leader-election/NRP_FD_Partitioning/./__lf_gendelay_1afb4029.h:7:3: error: redefinition of enumerator 'heartbeat'
  heartbeat,
  ^

etc.

Sorry, I don't have a more minimal example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants