PX4 / PX4-Autopilot

PX4 Autopilot Software
https://px4.io
BSD 3-Clause "New" or "Revised" License
8.53k stars 13.51k forks source link

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

Open davids5 opened 7 years ago

davids5 commented 7 years ago

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 commented 6 years ago

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 commented 6 years ago

This still an issue.

stale[bot] commented 5 years ago

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 commented 5 years ago

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 https://github.com/PX4/Firmware/pull/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 commented 5 years ago

@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[bot] commented 5 years ago

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

junwoo091400 commented 2 years ago

Is it still relevant @davids5 ?

davids5 commented 2 years ago

Yes. It is harder to separate it out