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

Make modm::delay compatible with std::chrono #374

Merged
merged 5 commits into from
Apr 6, 2020

Conversation

salkinium
Copy link
Member

@salkinium salkinium commented Apr 4, 2020

This adds std::chrono support for the modm::delay() blocking delay implementation.

The implementation duration_casts everything to microseconds by default, unless a specialization is provided:

  • modm::delay(std::chrono::nanoseconds), modm::delay_ns(uint32_t): Only implemented for Cortex-M, AVRs always delays 1us, Hosted casts to us (darwin/linux) or ms (windows).
  • modm::delay(std::chrono::microseconds), modm::delay_us(uint32_t): implemented for all, except windows (casts to ms).
  • modm::delay(std::chrono::milliseconds), modm::delay_ms(uint32_t): implemented for AVR and Hosted. Cortex-M casts to us.

In order to not go crazy from adding using namespace std::chrono_literals, I've added this to <modm/board.hpp> with a guard macro MODM_BOARD_DO_NOT_USE_LITERALS, so you can disable this behavior in your code. (It also adds using namespace modm::literals).

I have a usability question: Should modm::delayMilliseconds(uint16_t) be deprecated? During my refactoring, I had to add a lot of using namespace std::chrono_literals to driver methods, or construct the std::chrono::milliseconds(uint) in place. This is a little verbose. Perhaps the previous methods weren't such a bad idea? Should they perhaps be renamed to modm::delay_ms() for consistency?
Update: I've added modm::delay_[mun]s() functions as well, and it make perfect sense and the naming is good too.

TODO:

  • Better documentation
  • Add convenience delay_ms(uint), delay_us(uint), delay_ns(uint) methods?
  • Decide deprecation behavior
  • Double check if I got all delays right

cc @rleh @se-bi @strongly-typed @chris-durand

This PR is in preparation for #217.

@salkinium
Copy link
Member Author

I'm ready to merge this if there are no complaints. I don't think this is very controversial, except perhaps the deprecation of delayMilliseconds etc. Is this deprecation too annoying? I could also make it silently forward to the new functions and simply not document these methods, so that new users don't use them.

Copy link
Member

@chris-durand chris-durand left a comment

Choose a reason for hiding this comment

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

I agree on deprecating delayMilliseconds.

Maybe we should fix AVR delay_ns. The way it is implemented right now delay_ns(1000) is a no-op, which is kind of surprising. There is a new instrinsic in avr-gcc which could help. It only works on compile time known delays and with optimization turned on, though.
I have tried it and it seems to work fine here.

src/modm/architecture/interface/delay.md Outdated Show resolved Hide resolved
src/modm/platform/clock/stm32/rcc.cpp.in Outdated Show resolved Hide resolved
src/modm/platform/clock/stm32/rcc_impl.hpp.in Outdated Show resolved Hide resolved
src/modm/platform/core/cortex/module.md Outdated Show resolved Hide resolved
@salkinium
Copy link
Member Author

There is a new instrinsic in avr-gcc which could help. It only works on compile time known delays.

I know, and I explicitly chose the dynamic implementation with __DELAY_BACKWARD_COMPATIBLE__.
Unfortunately the implementation is horrible. Like they use multiplication and division with floating point numbers… holy shit:
https://www.nongnu.org/avr-libc/user-manual/delay_8h_source.html

void
_delay_us(double __us)
{
    double __tmp ;
    uint8_t __ticks;
    double __tmp2 ;
    __tmp = ((F_CPU) / 3e6) * __us;
    __tmp2 = ((F_CPU) / 4e6) * __us;
    if (__tmp < 1.0)
        __ticks = 1;
    else if (__tmp2 > 65535)
    {
        _delay_ms(__us / 1000.0);
    }
    else if (__tmp > 255)
    {
        uint16_t __ticks=(uint16_t)__tmp2;
        _delay_loop_2(__ticks);
        return;
    }
    else
        __ticks = (uint8_t)__tmp;
    _delay_loop_1(__ticks);
}

@salkinium
Copy link
Member Author

I need to implement this myself it seems, and then I can hack something much more useful together with __builtin_choose_expr and __builtin_constant_p.

@chris-durand
Copy link
Member

I just wanted to try the horrible runtime version in compiler explorer and found out _delay_us doesn't compile with -Os with a dynamic value with avr-gcc >= 5.

/opt/compiler-explorer/avr/arduino-1.8.9/hardware/tools/avr/avr/include/util/delay.h:276:40: error: __builtin_avr_delay_cycles expects a compile time integer constant

  __builtin_avr_delay_cycles(__ticks_dc);

                                        ^
Compiler returned: 1

for

int delay = 100;
int main() { _delay_us(delay); }

Maybe we never noticed that this doesn't work anymore because in modm all delays are static. I don't have a C++17 compatible avr-gcc here right now to test with modm.

@chris-durand
Copy link
Member

Oh, sorry. It works with #define __DELAY_BACKWARD_COMPATIBLE__. I defined it after including the delay header, stupid me.

@salkinium
Copy link
Member Author

This is what I came up with. I tried it with if constexpr, but that doesn't work, so the ?: operator is chosen: https://gcc.godbolt.org/z/W5LquY

I mean, the fastest this thing will ever go is 32MHz, which is 93ns per loop. At 8MHz, it's 375ns per loop. So the runtime version will be hopelessly over the top.

@chris-durand
Copy link
Member

This is what I came up with. I tried it with if constexpr, but that doesn't work, so the ?: operator is chosen: https://gcc.godbolt.org/z/W5LquY

Seems reasonable. The line _delay_loop_1(ns-1 / ns_per_loop); looks off.
Do you mean _delay_loop_1(ns / ns_per_loop - 1);?

I mean, the fastest this thing will ever go is 32MHz, which is 93ns per loop. At 8MHz, it's 375ns per loop. So the runtime version will be hopelessly over the top.

Don't worry, 3 cycles is still a lot better than the 32 bit float multiplication in avr-libc. If someone wants accurate nanosecond timings, they should choose a faster cpu.

This makes the :platform:clock module optional as it should be.
This prevents the automatic inclusion into <modm/platform.hpp> and thus
prevents include ordering issues.
@salkinium salkinium force-pushed the feature/delay_std_chrono branch 3 times, most recently from 16302fc to f6118fa Compare April 5, 2020 18:30
@salkinium
Copy link
Member Author

salkinium commented Apr 5, 2020

I've implemented a new AVR delay that uses __builtin_avr_delay_cycles for constant values, which is very accurate and for dynamic values a much faster nanosecond delay for AVR, which divides with shifts and thus has a max error factor of 2.

Testing suggests that dynamic ms and us delay are fairly accurate even over an accumulated period, but dynamic ns delay is not super accurate, but also not wildly off. I think this is fine for most applications.

@salkinium salkinium merged commit b010775 into modm-io:develop Apr 6, 2020
@salkinium salkinium deleted the feature/delay_std_chrono branch April 6, 2020 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants