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

✨ Newlib Stub Improvements #315

Merged
merged 17 commits into from
Oct 29, 2021
Merged

✨ Newlib Stub Improvements #315

merged 17 commits into from
Oct 29, 2021

Conversation

WillXuCodes
Copy link
Member

@WillXuCodes WillXuCodes commented Oct 10, 2021

Summary:

This PR intends to add proper functionality to _exit, and adds implementations for usleep and sleep.

Still needs testing and peer review... far from perfect in terms of implementation.

Closes #318

src/system/newlib_stubs.c Outdated Show resolved Hide resolved
}

int usleep( useconds_t period ) {
// naive blocking delay, still needs testing as I'm not sure if the tight loop will work.
Copy link
Member

Choose a reason for hiding this comment

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

Should be fine so long as vexSystemHighResTimeGet doesn't rely on vexBackgroundProcessing, which I doubt very much.

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, which is why I need to do testing in the near future.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why we're not calling pros::delay/calling into the scheduler (at least up to the accuracy of that)?

Copy link
Contributor

Choose a reason for hiding this comment

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

do we have access to a microsecond timer in freeRTOS? I don't think we've yet otherwise exposed the vexOS high res timer...

Copy link
Member Author

@WillXuCodes WillXuCodes Oct 17, 2021

Choose a reason for hiding this comment

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

The reasons for doing this are the following:

  1. As Hotel said, we don't have a uDelay function that delays in microseconds (*potentially something to work on).
  2. We do have microsecond timing in the form of getting the system time via pros::micros (I believe nick added it a couple months ago), which at the end of the day calls the same vexOS function.

Copy link
Member

@nathan-moore nathan-moore Oct 17, 2021

Choose a reason for hiding this comment

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

It looks like FreeRTOS works by counting interrupts , so a more precise delay implementation has trade offs. What I'm saying is that we likely want to sleep until we'd overstep the period on the next tick, then do this spinning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Transferred from Discord:

The plan is to now two kill birds with stone one by not just coupling these stubs to our scheduler, but also adding microsecond timing to our API. We could also add a delay in seconds to our API as well while we're at it.

include/api.h Outdated Show resolved Hide resolved
version Outdated Show resolved Hide resolved
@WillXuCodes
Copy link
Member Author

usleep confirmed to work, this was the test code:

void opcontrol() {
	pros::Controller master(pros::E_CONTROLLER_MASTER);
	while(true){
		printf("Delay Started\n");
		usleep(1000000);
		printf("Delay Finished\n");
	}
}

Ready for re-review

@nathan-moore
Copy link
Member

nathan-moore commented Oct 23, 2021

std::atomic<bool> flag;
void opcontrol() {
	flag = false;

	pros::Task task([]{flag = true; }, 3);

	while (!flag)
	{
		usleep(10000);
	}
}

Will currently deadlock.

Comment on lines 34 to 35
uint64_t endTime = vexSystemHighResTimeGet() + period;
while(vexSystemHighResTimeGet() < endTime) asm("NOP");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
uint64_t endTime = vexSystemHighResTimeGet() + period;
while(vexSystemHighResTimeGet() < endTime) asm("NOP");
uint64_t endTime = vexSystemHighResTimeGet() + period;
if (period > 1000)
{
task_delay(period / 1000);
}
while(vexSystemHighResTimeGet() < endTime) asm("YIELD");

Looks to be the best that we can do? You can also put a loop around the yield instruction, which would make things easier for the vex processor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I agree. I also realized I forgot to add an overflow check on the parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

std::atomic<bool> flag;
void opcontrol() {
	flag = false;

	pros::Task task([]{flag = true; }, 3);

	while (!flag)
	{
		usleep(10000);
	}
}

Will currently deadlock.

Yes. But attaching this to the scheduler as discussed would involve some extremely fundamental changes to the scheduler that are not worth it given how most people most likely will not use this function.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but my point is sort of that we can do better then what's currently here, since that's in the millisecond range. That and we'll likely want to document that this currently doesn't do any context switches to lower priority tasks when called with less then a millisecond resolution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, since our scheduler operates at millisecond resolution, we could just have it set errno to ENOSYS and be done with it. do we really need microsecond delays to work?

@WillXuCodes
Copy link
Member Author

After thinking about the best way to implement this, I've come to a compromise:

If the value is 1000 or above, it will call task_delay(period / 1000) and neglect the microsecond range.

If the value is less than 1000 microseconds, it will delay in the microsecond range but neglect interrupts.

@kunwarsahni01 kunwarsahni01 self-requested a review October 29, 2021 13:01
Copy link
Member

@kunwarsahni01 kunwarsahni01 left a comment

Choose a reason for hiding this comment

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

LGTM

@WillXuCodes WillXuCodes merged commit 760f2cf into develop Oct 29, 2021
@WillXuCodes WillXuCodes deleted the feature/more-newlib-stubs branch October 29, 2021 13:54
WillXuCodes added a commit that referenced this pull request Jun 11, 2022
* Added more newlib stubs

* Fixed compilation errors

* Fixed more compilation errors

* Attempted compilation fix (again)

* Added macro for conversion

* Started work on writing scheduler coupled micro wait

* Added scheduler based microsecond delay to API.

* Changed variable milliseconds to microseconds

* Fixed RTOS Header

* Fixed version/api files

* Fix Version

* Removed all microsecond delays from public API

* Revert Version, Revert main, revert back to naive delay

* Final Commit

* Changed usleep behavior.

* Changed NOP to Yield
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unable to compile cold package with arm-none-eabi 11
6 participants