Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Preambles of base classes are not included #1372

Closed
edwardalee opened this issue Sep 18, 2022 · 6 comments · Fixed by #2381
Closed

Preambles of base classes are not included #1372

edwardalee opened this issue Sep 18, 2022 · 6 comments · Fixed by #2381
Assignees
Labels
bug Something isn't working c Related to C target

Comments

@edwardalee
Copy link
Collaborator

The C code generator (at least) does not include the preambles of a base class, which causes code to fail to compile. Minimal test case:

target C;
reactor ImportedPreamble {
    preamble {=
        void test_function() {
            printf("Hello World\n");
        }
    =}
}
reactor Extension extends ImportedPreamble {
    reaction(startup) {=
        test_function();
    =}
}
main reactor {
    e = new Extension();
}

This causes a compile error:

ImportPreamble.lf:11:5: error: implicit declaration of function 'test_function' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
    test_function();
    ^
1 error generated.

Unfortunately, this is making it very hard to provide a clean robot interface for EECS 149 labs.

@edwardalee edwardalee added bug Something isn't working c Related to C target labels Sep 18, 2022
@edwardalee edwardalee self-assigned this Sep 18, 2022
@petervdonovan
Copy link
Collaborator

The following compiles and works fine for me. If we allow non-private methods, then we may as well use them. Christian commented a few years ago that

While I think the preamble is importan for imports and other things that require file scope, I don't think it should contain function definitions or other code.

target C;
reactor ImportedPreamble {
    method test_function() {=
         printf("Hello World\n");
    =}
}
reactor Extension extends ImportedPreamble {
    reaction(startup) {=
        test_function();
    =}
}
main reactor {
    e = new Extension();
}

I wouldn't want to belabor a point that has already been under discussion for years (#33), but it seems non-obvious that non-private methods (or preamble inheritance, given that we can declare state variables in preambles) is a good idea. Such things do break encapsulation. As in an OO language, composition can solve this (without using special constructs like methods or preambles):

target C
reactor ImportedPreamble {
    input call:bool
    reaction(call) {=
         printf("Hello World\n");
    =}
}
reactor Extension {
    ip = new ImportedPreamble()
    reaction(startup) -> ip.call {=
        lf_set(ip.call, true);
    =}
}
main reactor {
    e = new Extension();
}

@cmnrd
Copy link
Collaborator

cmnrd commented Sep 22, 2022

The semantics of preambles in combination with imports (and also inheritance) is still a mystery to me. That's also why #33 is still open. However, I think the issue here is that we never tried to modularize the generated code. I am not sure if it changed in the meantime (AFAIK it did not), but back then the visibility of preamble code completely depended on the order in which the code generator would place reactors in the generated C file. This means that some reactors see preambles that they shouldn't see and some don't see preambles that we might expect them to see.

@cmnrd
Copy link
Collaborator

cmnrd commented Sep 22, 2022

For the robot interface, it might actually make sense to place the functions in separate header and source files. This would give a cleaner interface to be used by the robot control reactors.

@edwardalee
Copy link
Collaborator Author

For my particular problem with the robot, I have done what @cmnrd suggests and put the functions in a separate .h/.c file. However, the bug still stands, because the #include has to be put in a preamble, and the preamble will not be inherited.

@DNDEMoto
Copy link

I'm facing the same problem in python targets in LF 0.8.1.

target Python;
reactor RosNode {
    preamble {=
        def initiation() {
            printf("Hello World\n")
        }
    =}
}
reactor PIController extends RosNode {
    reaction(startup) {=
        self.initiation()

    =}
}
reactor PIDController extends RosNode {
    reaction(startup) {=
        self.initiation()

    =}
}
main reactor {
    e = new PIController();
}

An error will occur when executed.

AttributeError: '__picontroller' object has no attribute 'initiation'

In my use case (the code above is a minimal sample of this use case),
I want to use a reactor with preamble as a so-called abstract class. then, by inheriting this reactor, common functions and member variables will be available from subclasses.

I would like to know if there are any workarounds or revisions planned for this issue.

@edwardalee
Copy link
Collaborator Author

This is fixed in PR #2381 .

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working c Related to C target
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants