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

HRT common code ( and derivatives like Tunes library implementation HRT uses) #8099

Open
davids5 opened this issue Oct 9, 2017 · 8 comments

Comments

@davids5
Copy link
Member

davids5 commented Oct 9, 2017

Now that there are 4 chip architectures stm32, stm32F7, Kinetis and samv7

There is a need to isolate the stm32 arch specific code from the arch agnostic code for drivers (hrt in this case) and derivatives. Such as Tunes library implementation .

@PX4BuildBot
Copy link
Collaborator

Hey, this issue has been closed because the label status/STALE is set and there were no updates for 30 days. Feel free to reopen this issue if you deem it appropriate.

(This is an automated comment from GitMate.io.)

@simonegu
Copy link
Contributor

simonegu commented Mar 6, 2018

This still an issue.

@simonegu simonegu reopened this Mar 6, 2018
@dagar dagar removed this from the Release v1.8.0 milestone May 4, 2018
@stale
Copy link

stale bot commented Jan 29, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@davids5
Copy link
Member Author

davids5 commented Jan 31, 2019

We need a pass on the htr to do some clean up and document all the interrupt states in the call tree.
then build the unified driver with arch specifics like the tone_alarm.

fyi: @mcsauder see #11351 for the 32 bit timer version.

I think this needs a critical section

hrt_call_delay(struct hrt_call *entry, hrt_abstime delay)
{
	entry->deadline = hrt_absolute_time() + delay;
}

There is also this that makes no sense

/**
 * Store the absolute time in an interrupt-safe fashion
 */
hrt_abstime
hrt_store_absolute_time(volatile hrt_abstime *now)
{
	irqstate_t flags = px4_enter_critical_section();

	hrt_abstime ts = hrt_absolute_time();

	px4_leave_critical_section(flags);

	return ts;
}

@LorenzMeier, @bkueng, @dagar how would you feel about changing the new hrt_elapsed_time_atomic to be done differently or removed because hrt_absolute_time already has has a critical section.
then:
hrt_absolute_time(void) should become _hrt_absolute_time(const volatile hrt_abstime *offset)
hrt_absolute_time(void) wraps and calls _hrt_absolute_time(nullptr)

Then hrt_elapsed_time_atomic(const volatile hrt_abstime *then) will use _hrt_absolute_time(then) and it its critical section if (my_offset~==nullptr) do the math and return that value.

The additional overhead is 1 pointer null check and one register load for the call with null ptr and for the call with an offset it is just the math as opposed to 2 cs prologues and epilogues which is many more instructions that add not value.

@bkueng
Copy link
Member

bkueng commented Feb 4, 2019

@davids5 given that hrt_elapsed_time_atomic is almost never used, I don't think it's worth optimizing. On the other hand hrt_absolute_time is used all over the place, called at high frequency, and therefore worth optimizing every bit.

@stale stale bot removed the Admin: Wont fix label Feb 4, 2019
@stale stale bot unassigned simonegu Jun 24, 2019
@stale stale bot closed this as completed Jul 8, 2019
@PX4 PX4 deleted a comment from stale bot Jul 10, 2019
@PX4 PX4 deleted a comment from stale bot Jul 10, 2019
@julianoes julianoes reopened this Jul 10, 2019
@stale stale bot removed the Admin: Wont fix label Jul 10, 2019
@stale
Copy link

stale bot commented Oct 8, 2019

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Oct 8, 2019
@junwoo091400
Copy link
Contributor

Is it still relevant @davids5 ?

@stale stale bot removed the stale label Jul 27, 2022
@davids5
Copy link
Member Author

davids5 commented Jul 27, 2022

Yes. It is harder to separate it out

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

No branches or pull requests

7 participants