-
Notifications
You must be signed in to change notification settings - Fork 24
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
Changes to facilitate arduino-cli support #138
Conversation
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.
LGTM!
struct timespec { | ||
long long tv_sec; /* seconds */ | ||
long tv_nsec; /* and nanoseconds */ | ||
}; |
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 is this used for?
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.
AVR doesn't have timespec to hold the time when you first load the program. MBED does, so I just made the custom timespec struct to hold it.
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.
Hm. I don't understand what you mean. Is there some external dependency we are linking with that needs this struct defined?
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'm also a bit puzzled by this code. Do we actually need it?
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.
Inside of reactor_common.c, when we initialize we have the following set of code:
LF_PRINT_DEBUG("Start time: " PRINTF_TIME "ns", start_time);
struct timespec physical_time_timespec = {physical_start_time / BILLION, physical_start_time % BILLION};
printf("---- Start execution at time %s---- plus %ld nanoseconds.\n",
ctime(&physical_time_timespec.tv_sec), physical_time_timespec.tv_nsec);
This is the only place the timespec is referenced for the Arduino lifecycle, but I myself don't know what purpose this has.
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.
For MBED systems like the BLE Sense Boards, timespec struct is already defined, but AVR doesn't have it formally defined. To prevent the system from breaking, I simply hardcoded the timespec struct. That said, I still don't know the underlying purpose of the struct if we simply read to and from the struct and have it get discarded later?
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.
Looks like a remnant of early code that used stuct timespec
directly. If physical_time_timespec
isn't used anywhere else and there is not other use of struct timespec
then I suggest removing it.
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 looks like the federated reactor still needs timespec and I'm currently working on federated support for Arduino so I think it's best to keep it.
Good work. The test failure is related to #133. I don't know when this started or what the cause is. We should look into it at some point. But just re-run the failing tests and it always just works |
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.
LGTM!
One comment I had for @erlingrj is the reason behind removing using Arduino delay functionality? I must have missed when this was taken out. |
Could you clarify what is the delay functionality? Do you mean using busy-wait with lf_clock_gettine rather than delay for sleep? That is for supporting physical actions that interrupt the sleep |
Minor changes are needed in reactor-c to faciliate arduino-cli support. Compiles successfully locally with changes in LFC