-
Notifications
You must be signed in to change notification settings - Fork 30
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
Clean main and init files from different architectures #151
Conversation
i test the FreeRTOS now. |
so,
now i changed my forte Component in ESP32 to this: and it compiles again. i will now test if it is working. |
I changed the interface to reflect properly what the functions are doing and more closely related to the Device functions. Also, ideally you should be able now to have everything you need by including the file
|
which is nice, but not so practical.
|
I see, but the The way you use it hasn't change much, just the signatures of the methods and the |
Just letting you know that I wouldn't be able to review this PR with regard to the Zephyr port in the near future. I'm sorry. |
wrong. it is vanilla FreeRTOS:
true
|
To compile with a C (not C++) main source file:
|
src/arch/c_interface/forte_c.cpp
Outdated
|
||
unsigned const int cgForteDefaultPort = 61499; | ||
static const unsigned int scForteDefaultPort = 61499; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the coding guidelines doesn't know the prefix sc for static const, only cg for const global.
While file-scope globals theoretically have less impact, they are only an extern
keyword, used by someone else, away being visible in other compilation units as well.
If we want to code the difference of usage into the name, I would rather suggest cf for "constant file" instead of
sg "static const", as static const doesn't imply anything than the variable is some sort of global variable and lives in this compilation unit. "cf" would also be more in line with "cg" for const global,
i will mark this only here, to not split up the discussion on several review comments, but my comment is valid for all "sc" changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also recommend to add the "U" suffix to the number, as its an unsigned constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would strongly suggest to use an unnamed namespace in C++ instead of static
linkage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with the unnamed namespace
since it's more c++ style. Regarding the naming convetion, since there's no defined convention for constant variables in unnames namespaces I didn't add any prefix to avoid having a "convention" only in that file which is not documented anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the unnamed namespace solution. Perhaps it wasn't that clear in my comment, if we feel we need another prefix to show the intent/usage we could add one to the coding guidelines.
I wasn't suggesting using another undocumented prefix in favor of another
src/arch/main.cpp
Outdated
sleep(3); | ||
DeviceStatus::startup(); | ||
#endif | ||
struct callOnExit { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open for discussion, but do we really want to use structs as more than plain data containers? @azoitl @mx990
From my perspective, its would be clearer to use classes everywhere we need more than a collection of data entries, and reserve the use of structs exactly for that, plain data containers.
My rationale for this is, that you know what to expect if you see class or struct that way, without needing to check the code.
The cost is quite low, only adding public:
as additional line here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not change CForteArchitecture::deinitialize
to a destructor and use a local variable of type CForteArchitecture
to avoid the additional struct? Alternatively, there is std::atexit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the std::atexit
since it applies in the context of the main
function in a c++ environment.
src/arch/main.cpp
Outdated
~callOnExit() { onExit(); } | ||
}; | ||
|
||
callOnExit{CForteArchitecture::deinitialize}; | ||
|
||
hookSignals(); | ||
|
||
const char *pIpPort = parseCommandLineArguments(argc, arg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please remove the prefixed p
here, as the coding guidelines abolished the use of Hungarian notation for most parts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
#include "arch/forte_specific_architecture.h" | ||
|
||
/* CG: testing */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this comment mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. I only moved things around, so this came from the original code
tests/arch/forte_c_test.cpp
Outdated
|
||
// forteStartInstance | ||
// invalid instance result | ||
BOOST_CHECK_EQUAL(forteStartInstance(61499, nullptr), FORTE_WRONG_PARAMETERS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please use BOOST_TEST
instead of BOOST_CHECK
, as it results in better test failure output and is nicer to read (at least in my opinion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the better output look like? In my opinion, if this test fails you get the output of the actual vs expected value which helps you understand why the test failed, which I believe you don't have if you just use BOOST_TEST
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BOOST_TEST handles expressions and produces output including the full expression with individual values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, but I get the exact same output. If I have invalid cases as in:
BOOST_CHECK_EQUAL(forteGlobalInitialize(validArgc, validArgV), 1);
BOOST_TEST(forteGlobalInitialize(validArgc, validArgV) == 1);
I get
~/4diac-forte/tests/arch/forte_c_test.cpp(56): error: in "forte_c_interface": check forteGlobalInitialize(validArgc, validArgV) == 1 has failed [0 != 1]
~/4diac-forte/tests/arch/forte_c_test.cpp(57): error: in "forte_c_interface": check forteGlobalInitialize(validArgc, validArgV) == 1 has failed [0 != 1]
maybe I missed something, but I don't see any better output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this simple case, the output seems to be the same, but in my personal experience of writing tests in the past, i found BOOST_TEST generating better test output, but your mileage may vary.
Additionally as mentioned in my first comment about this, one of my reasons to stick to BOOST_TEST
is the better readability of the check itself.
Just to explain my reasoning, with BOOST_CHECK_EQUAL
I first get the information the parameters are to be compared for equality, and only after I analyse the parameters (assuming reading from left to right), needing to hold the information of what kind of comparison needs to be performed in my head.
For BOOST_CHECK_GT
etc. its even worse, as I need to know the order of operators and keep them in mind while checking the parameter expression.
On the other hand, BOOST_TEST
let you write the expression to be checked in native C/C++ and I can see them in "natural" order of the language.
In addition, I find it easier to only need to know (usually) one test macro, and ignoring it as code noise, focusing on the expression itself.
Also as said above, this is subject to personal preference, and open to discussion of course.
We should also write the results down into our coding guidelines, so the consensus is documented and publicly available in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to add, that according to the boost sources, the BOOST_CHECK* macros seem to be deprecated, see
https://github.com/boostorg/test/blob/b0127f48d23b94387c9e57b6b481f9119a3a25db/include/boost/test/tools/interface.hpp#L273-L308. When disabling the "old tools" by defining BOOST_TEST_NO_OLD_TOOLS, they simply forward to BOOST_TEST_CHECK (as shown in the linked snippet and the output above then becomes identical).
Thanks for finding this, I thought I have read that BOOST_CHECK* have been deprecated, but couldn't find that anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very interesting. indeed the output is better. I'll change all to BOOST_TEST
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. One thing that I realized while changing now to BOOST_TEST
is that you need the <<
operator for the type you're comparing.
For example,
BOOST_TEST(EMGMResponse::Ready == resource->writeValue(command.mFirstParam, "1", false));
does not compile and it requires
std::ostream& operator<<(std::ostream& os, const EMGMResponse& val ){
// ...
return os;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boost.test gives you the possibility to add a helper function (https://github.com/eclipse-4diac/4diac-forte/blob/develop/tests/forte_boost_output_support.h) instead of the stream operator, in cases where it doesn't make sense for the type itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow! I'm completely out of touch regarding boost
. I updated accordingly
@@ -1,5 +1,5 @@ | |||
/******************************************************************************* | |||
* Copyright (c) 2019 fotiss GmbH | |||
* Copyright (c) 2024 Jose Cabral |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if changing the copyright here from Fortiss GmbH to exclusively Jose Cabral is valid here, as much of the original code is still present.
Even if you wrote the code originally, it seems you transferred the copyright to Fortiss back then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a correction from a commit from a couple of months ago actually. The original file is not from 2019 and not done at fortiss, but by me this year
tests/core/trace/ctfTracerTest.cpp
Outdated
@@ -105,7 +105,7 @@ BOOST_AUTO_TEST_CASE(sequential_events_test) { | |||
command.mFirstParam.clear(); | |||
command.mFirstParam.pushBack(counterInstanceName); | |||
command.mFirstParam.pushBack(g_nStringIdPV); | |||
BOOST_CHECK(EMGMResponse::Ready == resource->writeValue(command.mFirstParam, CIEC_STRING(std::string("1")), false)); | |||
BOOST_CHECK(EMGMResponse::Ready == resource->writeValue(command.mFirstParam, std::string("1"), false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use BOOST_TEST
and std::string literals here "1"s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/arch/c_interface/forte_c.cpp
Outdated
|
||
unsigned const int cgForteDefaultPort = 61499; | ||
static const unsigned int scForteDefaultPort = 61499; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would strongly suggest to use an unnamed namespace in C++ instead of static
linkage.
src/arch/main.cpp
Outdated
sleep(3); | ||
DeviceStatus::startup(); | ||
#endif | ||
struct callOnExit { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not change CForteArchitecture::deinitialize
to a destructor and use a local variable of type CForteArchitecture
to avoid the additional struct? Alternatively, there is std::atexit.
src/arch/forte_architecture.h
Outdated
#include "forteinit.h" | ||
#include "startuphook.h" | ||
|
||
template<typename SpecificArchitectureT> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems overkill to me to use a template parameter on top of conditional compilation. What is the benefit instead of just using CForteSpecificArchitecture
directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main motivation was usability. When you are using the architecture inside forte, the specifics of the architecture should not be visible to the user. By having a "general" architecture, we can define a proper interface with a clear documentation of what it does.
Specific architectures might add variables or other private methods which might distract the user.
Also, when creating a new architecture, you should only look at the "general" architecture and nothing else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I worded that poorly. I am not against splitting this into a general and specific part, far from it. I am merely wondering why we need a template parameter instead of just using CForteSpecificArchitecture
inside CForteGeneralArchitecture
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I completely missed that. You're right. It's updated now
src/arch/c_interface/forte_c.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file does not seem to be architecture-dependent. Should this not rather live directly in src
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agrre, but before moving it I would leave that for discussion mainly because there was a similar case somewhere up there in the comments regarding the startuphook
file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently we have everything that is used to start 4diac FORTE on a specific device in the arch folder. Therefore I would like to keep it here. Because when I'm porting 4diac FORTE to a new device I would look here first.
Duplicated code was removed and a centralized entry point to forte is given (when possible).