RFC: use Rust edition 2018 and add timer proxy

[geomatsi]

Hi @Rahix

Here is a PR that includes two patches:

• switch to Rust 2018 and update deps
• add Timer proxy

Regards,
Sergey

[Rahix]

Hi Sergey,

thanks for your PR! The first patch looks great, I'd like to merge that independently of the second one.

With the timer proxy, I am not sure if that is a good idea. Consider two drivers independently setting up the count-down in parallel and then waiting on it. This will lead to the wait() returning early (or late) for one of the two, which is definitely not the intended behavior:

    +------+          +-----+      +------+
    |Task A|          |Timer|      |Task B|
    +------+          +-----+      +------+

                      c = 0

    t.start(100)
                \---> c = 100
                                    t.start(10)
                      c = 10 <-----/
                                    t.wait()
    t.wait()
                      c = 9
                      c = 8
                      ...
                      c = 1
    /* Also expires   c = 0         /* Expires after 10 */
     * after 10
     * instead of 100
     */

What is your exact use-case for the timer-proxy? I believe we have (again) hit the deeper lying problem, of which shared-bus attempts to solve a subset. The root-problem here is the need to share peripherals between multiple drivers. This can be necessary in a lot of cases:

• Buses, for which shared-bus is a solution
• Timer Peripherals, as you pointed out
• GPIOs or more generally pins, for multiplexing
• Delay implementations
• Probably a lot more

The problem is unique to Embedded Rust in a way because we enforce these strict ownership rules. In C, sharing was just done without much thinking, which lead to a great number of bugs. A common "solution" is to just add a mutex for the peripheral in question and hope this is enough. For a simple bus it is, which is why this approach works for shared-bus. For a timer on the other hand, it is not as the timer keeps (mutable) state between mutex locks.

Please don't misunderstand: I definitely see the need for sharing a timer peripheral between multiple tasks / drivers. I see two solutions for this:

A) Global Mutex

Stuffing the entire timer into a mutex, without a proxy. This would ensure each user locks the entire timer for the duration where it needs the peripheral. It would definitely be a 100% safe implementation but has the huge drawback that there cannot be parallel count-downs from multiple users at the same time.

B) shared-timer

The more flexible solution would be to create a new crate, similar to shared-bus which contains an implementation of a timer manager. This manager needs to keep track of all ongoing count-downs / waits and properly set up the underlying timer to serve correct values to each one of them. I believe such an implementation is non-trivial but might be valuable for the embedded-hal ecosystem.

To allow writing such a crate, we would first need a common mutex trait. I have started a PR regarding this (rust-embedded/embedded-hal#132). Unfortunately I did not have much time in the last months to continue working on it but this will hopefully change in the next weeks.

---

What is your opinion on all this? What was the original use-case which motivated you to create the current implementation?

[geomatsi]

Hi @Rahix
Thanks for your feedback! I will be offline during the weekend, so I will take a detailed look at your comment early next week. I agree that the timer patch needs more discussion. Besides, I plan to submit an RFC PR with a draft implementation of shared ADC in order to get feedback.

In brief, my primary use-case for shared timer is to use it for clock generation in bitbang-hal. Example (single shared timer for bitbang i2c and spi) can be found here

Regards,
Sergey

[geomatsi]

Hi @Rahix

1. Concerning Rust 2018 update...
   Let me know if I should submit the first commit as a separate PR.

2. Concerning shared timer...
   According to your classification, the purpose was to implement shared Delay. To be more specific, to be able to share the same hardware timer between multiple instances of bitbang-hal for clock generation. Note that the issue that you described can be addressed using start method of the CountDown trait: users may restart hardware timer using required frequency each time they get access to it.

Regards,
Sergey

[Rahix]

Hi Sergey,

Let me know if I should submit the first commit as a separate PR.

Please do!

According to your classification, the purpose was to implement shared Delay. To be more specific, to be able to share the same hardware timer between multiple instances of bitbang-hal for clock generation. Note that the issue that you described can be addressed using start method of the CountDown trait: users may restart hardware timer using required frequency each time they get access to it.

I don't think so. With shared peripherals you have to think in a preemptive context: At any time an interrupt might be triggered and modify the peripheral before returning execution in your routines. This is perfectly safe with the abstraction shared-bus provides.

Take another look at the diagram I drew. Both tasks call t.start() before making use of the timer. But immediately after task A does so, it gets preempted by task B which calls t.start() as well, thus overriding the timer value A is expecting.

This kind of race-condition is what Rust is trying to prevent with its ownership system so shared-bus should also only expose an API which can uphold these guarantees. The two proposed solutions above are the only ones I can think of which deal with this situation the correct way.

---

Looking at bitbang-hal (the serial implementation to be specific), t.start() is only called in one place: here. The "user" of the timer (ie. the bitbang serial driver) doesn't ever restart the timer and would thus get confused if another timer-proxy would do so.

I think you are trying to use shared-bus for something it is not meant to be used for: To mutually exclusively share a peripheral between multiple drivers. In a sense, not to allow safe preemptive multitasking where multiple tasks need access to the peripheral, but to allow reusing the same timer for different purposes in the same task:

fn task_foo() {
    let timer = SharedTimer::new(...);

    let mut serial = Serial::new(..., timer);
    let mut i2c = I2c::new(..., timer);

    /* Now use the serial port for some time, but **NOT** the i2c */
    timer.start(115200.hz());
    writeln!(&mut serial, "Hello!\r\n");

    /* Now use the i2c bus for some time */
    timer.start(400.khz())
    i2c.write(0x3A, &[0xDE, 0xAD, 0xBE, 0xEF]);
}

In this example, there are no race-conditions possible, but Rust can't know that. IMO this is a shortcoming of the definition of the timer::CountDown trait on one hand and the definition of the Serial implementation in bitbang-hal in the other. There is also no safety in the above example that a user won't forget to reconfigure the clock:

fn task_foo() {
    let timer = SharedTimer::new(...);

    let mut serial = Serial::new(..., timer);
    let mut i2c = I2c::new(..., timer);

    timer.start(400.khz())
    i2c.write(0x3A, &[0xDE, 0xAD, 0xBE, 0xEF]);

    // Forgot to add the t.start() here, oops
    // timer.start(115200.hz());

    // Now the serial is sending way too fast and the other end won't be able
    // to decode the data
    writeln!(&mut serial, "Hello!\r\n");

}

Instead, in a safe implementation it would probably look more like this:

    let timer = ...; /* No SharedTimer this time */
    let serial = Serial::new(..., 115200.hz());
    let i2c = I2c::new(..., 400.khz());

    /* Acquire the timer for serial for some time: */
    let mut serial = serial.take_timer(timer);
    /* Timer is now moved into the serial driver,
       Rust will prevent access from anyone else */

    writeln!(&mut serial, "Hello!\r\n");
    /* Get back the timer. After this, the serial can no longer be written to */
    let (serial, timer) = serial.release_timer();

    /* Move timer into i2c driver, to allow accessing the bus */
    let mut i2c = i2c.take_timer(timer);

    i2c.write(0x3A, &[0xDE, 0xAD, 0xBE, 0xEF]);
    /* Stop using i2c */
    let (i2c, timer) = i2c.release_timer();

This approach is called the type-state pattern. I have linked it to a more detailed example.

[geomatsi]

Hi @Rahix

Ok, I see the point now. Suggested timer proxy indeed can not be used to clock bitbang-hal even if we modify its spi and i2c implementations to set proper timer frequency before each new transaction (btw, it should be done anyway, updated my TODO). Unlike i2c or spi shared-bus proxies, in the case of timer our code can be preempted after any simulated clock cycle rather than after the completion of transaction as a whole.

In my particular case I have a single hardware timer suitable for clocking multiple bitbang-hal instances. Since shared-bus timer proxy is not an option, I have to use some kind of wrappers on top of device drivers and implement either type-safe patter or just disable interrupts for each bitbang-hal transaction sharing hardware timer using something like RefCell.

P.S.
So I will close this PR and re-submit Rust 2018 update separately. Lets move on to #6 :)

Regards,
Sergey