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

Changes to facilitate arduino-cli support #138

Merged
merged 2 commits into from
Jan 9, 2023
Merged

Conversation

arengarajan99
Copy link
Collaborator

Minor changes are needed in reactor-c to faciliate arduino-cli support. Compiles successfully locally with changes in LFC

Copy link
Contributor

@petervdonovan petervdonovan left a comment

Choose a reason for hiding this comment

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

LGTM!

core/platform/lf_arduino_support.c Outdated Show resolved Hide resolved
core/platform/lf_arduino_support.c Outdated Show resolved Hide resolved
Comment on lines +99 to +102
struct timespec {
long long tv_sec; /* seconds */
long tv_nsec; /* and nanoseconds */
};
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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?

Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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?

Copy link
Member

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.

Copy link
Collaborator Author

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.

@erlingrj
Copy link
Collaborator

erlingrj commented Jan 9, 2023

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

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!

@arengarajan99
Copy link
Collaborator Author

One comment I had for @erlingrj is the reason behind removing using Arduino delay functionality? I must have missed when this was taken out.

@arengarajan99 arengarajan99 merged commit 66096e0 into main Jan 9, 2023
@erlingrj
Copy link
Collaborator

erlingrj commented Jan 9, 2023

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

@lhstrh lhstrh changed the title Update Reactor-C with changes to facilitate arduino-cli support Changes to facilitate arduino-cli support Jan 26, 2023
@lhstrh lhstrh added the feature New feature label Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants