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

Removed redundant code #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mdaiter
Copy link

@mdaiter mdaiter commented Jan 31, 2016

There was redundant code within the if statement of the WriteBit function. Tried to reduce redundancy for compiler output's sake.

@PaulStoffregen
Copy link
Owner

Have you tested the timing?

This code is fewer lines of source, and may compile to few bytes, but it does more work within the timing critical sections. The original was designed to push all the extra stuff outside of the interrupt disable regions.

@mdaiter
Copy link
Author

mdaiter commented Jan 31, 2016

Haven't tested the timing due to not having a board. However, it's just a boolean comparison. Would it really affect timing that much?

@PaulStoffregen
Copy link
Owner

Yes, OneWire is timing critical. Especially on slow 8 bit processors like AVR, these small details matter.

@orgua
Copy link

orgua commented Jul 16, 2017

yes, timing is critical, but the master is mostly dictating the timings. and a 1 MHz processor can waste a cycle and just spend 1µs on it. still ok. proposal to stay outside of atomic-code:

    const uint8_t time_high = uint8_t(value ? 10 : 60);
    const uint8_t time_low  = uint8_t(value ? 55 : 5);

    noInterrupts();
    DIRECT_WRITE_LOW(_baseReg, _bitMask);
    DIRECT_MODE_OUTPUT(_baseReg, _bitMask);    // drive output low
    delayMicroseconds(time_high);
    DIRECT_WRITE_HIGH(_baseReg, _bitMask);    // drive output high
    interrupts();
    delayMicroseconds(time_low);

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.

None yet

3 participants