Code review on embedded project : 🚗

Edit: Absolutely any criticism over my Rust code is more than welcome as I choose this project to learn Rust better :slight_smile:

Hello everyone,

The robot (repository link here) portage from C to Rust is going pretty well. I had humongous help from Rahix (the avr-hal creator) and Ana Hoverbear, and the embedded community.

here_comes_tiger_2_17mbn(1)

There is still those hardware problems for which I really need guidance:

  • when connecting the board the code is not running right away and can need restarting,
  • one of the wheels sometimes needsto get a nudge after I repaired the brushes. When I press on the top of the dc motor it also stops running completely!
  • the motor driver OR the MCU (I'm uncertain) is beeping like a baby bird calling its mother :baby_chick:. It's a continuous high pitched sound. Feels like MHSS (misshandled-hardware social services) will be there any minute.

Questions:

  1. why do I need to restart the board?
  2. what is causing the motor dysfunction? Do the brushes need to be replaced completely with new metallic pieces?
  3. what can cause the beeping sound? Is it likely to be the code?
6 Likes

Hello everyone,

I could discard problem 3 and 2 as mechanical problems. I tried code not written by me and I could see the same behaviour. I cannot reproduce problem 1 neither.

So allow me to change my request :laughing: :yum:!

Is there anyone willing to share with me their wisdom with a traditional code review? I want the code to be as respectful to the Rust convention as possible, and as close to the Rust spirit as possible. Also, if you see big NO-NO, please tell me.

I want to make this an easy Rust project to help beginners like myself to transition from Arduino to Rust.

Thank you in advance for reading and your time. Of course even partial advice is very much appreciated :slight_smile:

Main:

//! ## Classical obstacle avoiding robot with Rust
//! This project takes and Arduino classic and tries to port it to
//! Rust as a beginner project.
//! It uses avr-hal crate for abstraction.
//! For hardware it uses a simple HC-SR04 sensor, an L298N motor driver and a SG90
//! servo motor (details in the Readme).
//! The sensor is reading distance every > 100ms and the robot should take appropriate
//! action if an obstacle is detected.

// Macros to inform rust that the project will not use
// main and the standard library (lib std adds a layer to build the usual functions.)
#![no_std]
#![no_main]

// Pull in the panic handler from panic-halt
extern crate panic_halt;

use arduino_uno::hal::port::mode::Floating;
use arduino_uno::prelude::*;

use crate::motors::{go_backward, go_forward, stop, turn_left, turn_right};
use crate::sensor::{return_distance, SensorUnit};
use crate::servo::ServoUnit;
use arduino_uno::pwm;

mod motors;
mod sensor;
mod servo;

const WAIT_BETWEEN_ACTIONS: u16 = 1000u16;
const MINIMAL_DISTANCE: u16 = 10u16;
const ACCEPTABLE_DISTANCE: u16 = 10u16;
// creates the main function
// attribute macro -> transforms the next as the entry point
// "!" is a never type. It informs nothing should return from the main function.
#[arduino_uno::entry]
fn main() -> ! {
    // we acquire first a singleton of all the peripherals (everything inside the MCU)
    // more information on raw registers abstraction here:
    // https://docs.rs/avr-device/0.2.1/avr_device/atmega328p/struct.Peripherals.html
    let dp = arduino_uno::Peripherals::take().unwrap();

    let mut delay = arduino_uno::Delay::new();

    // all the ports are collapsed into the variable pins
    // docs on all pins: https://rahix.github.io/avr-hal/arduino_uno/struct.Pins.html
    // by default all pins are configured as Inputs and Floating
    // (pull up is to avoid undefined state. For arduino boards (5V), pull-up will allow up or down.
    let mut pins = arduino_uno::Pins::new(dp.PORTB, dp.PORTC, dp.PORTD);

    // this is the console. To see the output do (on mac)
    // screen /dev/tty/<your_tty_here> 57600
    // ls /dev/tty* | grep usb --> get the usb connected
    // 57600 is the baud rate
    let mut serial = arduino_uno::Serial::new(
        // protocol to communicate bytes in 2 directions
        // USART0 is moved to serial, serial becomes the new owner
        // https://rahix.github.io/avr-hal/atmega328p_hal/usart/struct.Usart0.html
        dp.USART0,
        // the values below correspond to :
        // rx: receive pin (hardwired into the MCU)
        // tx : PD1 is the "hardcoded output"
        // the ownership is moved by writing explicitely input, output is enforced at compile time,
        pins.d0,
        pins.d1.into_output(&mut pins.ddr),
        // other well known baud rates are possible (9600)
        57600,
    );

    // initialisation of timer 1 :  we write over and set prescaler to 64
    // (1/(16e6/64)) * 2^16 (size of register) ~> takes 262 ms for a cycle
    // timer1 is shared with the sensor unit
    let timer1 = dp.TC1;
    timer1.tccr1b.write(|w| w.cs1().prescale_64());

    let mut timer2 = pwm::Timer2Pwm::new(dp.TC2, pwm::Prescaler::Prescale1024);

    let mut pd3 = pins.d3.into_output(&mut pins.ddr).into_pwm(&mut timer2);
    pd3.enable();
        let mut servo_unit = ServoUnit{
        servo: pd3,
    };

    // servo is best set as a struct for clarity, it will be send to

    let mut sensor_unit = SensorUnit {
        // We do not use pin 13, because it is also connected to an onboard LED marked "L"
        // ownership issues: we are moving the pins.d13 into first, the function into_output
        // second, into led. It needs the ddr register for configuration
        // (DDRx are used to configure the respective PORT as output/input)
        trig: pins.d12.into_output(&mut pins.ddr),
        // floating input is set by default so we can configure echo without ddr
        echo: pins.d11,
        timer: timer1,
    };

    // downgrading the pins allow to put them in an array and simplify functions:
    // according to docs : Downgrade this pin into a type that is generic over all pins.
    let left_forw = pins.d4.into_output(&mut pins.ddr).downgrade();
    let left_back = pins.d5.into_output(&mut pins.ddr).downgrade();
    let right_forw = pins.d6.into_output(&mut pins.ddr).downgrade();
    let right_back = pins.d7.into_output(&mut pins.ddr).downgrade();

    // we have now mutable wheels that can be sent to motor functions
    let mut wheels = [left_forw, left_back, right_forw, right_back];

    // the car is always going forward (and printing distance to console if connected to screen)
    // until it meets an obstacle.
    'outer: loop {
        servo_unit.look_front();
        go_forward(&mut wheels);

        let value = return_distance(&mut sensor_unit);
        ufmt::uwriteln!( & mut serial, "Hello, we are {} cms away from target!\r", value).void_unwrap();

        if value < MINIMAL_DISTANCE {
            // the 'obstacle_avoidance loop. I would like to name it, but the compiler will complain :)
            loop {
                stop(&mut wheels);

                servo_unit.look_right();
                let value_right = return_distance(&mut sensor_unit);
                ufmt::uwriteln!( & mut serial, "On right, we are {} cms away from target!\r", value).void_unwrap();

                delay.delay_ms(WAIT_BETWEEN_ACTIONS);

                servo_unit.look_left();
                let value_left = return_distance(&mut sensor_unit);
                ufmt::uwriteln!( & mut serial, "On left, we are {} cms away from target!\r", value).void_unwrap();

                delay.delay_ms(WAIT_BETWEEN_ACTIONS);

                if (value_left > value_right) && value_left > ACCEPTABLE_DISTANCE {
                    turn_left(&mut wheels);
                } else if (value_right > value_left) && value_right > ACCEPTABLE_DISTANCE {
                    turn_right(&mut wheels);
                } else {
                    go_backward(&mut wheels);
                    delay.delay_ms(WAIT_BETWEEN_ACTIONS);
                    turn_right(&mut wheels);
                }
                continue 'outer;
            }
        }
        // the sensor needs to wait approximately 60 ms between two waves.
        // we ensure that by waiting while the register reaches 25000
        // one count == 4 us, and 4us*0.000004 == 100 ms
        while sensor_unit.timer.tcnt1.read().bits() < 25000 {}

        // I honestly forgot why I print that twice...
        ufmt::uwriteln!( & mut serial, "Hello, we are {} cms away from target!\r", value).void_unwrap();
    }
}

MOTORS:

//! ### The Motors Module
//! Handles the movement functions.
//! It unpacks the wheel pins in an array.

use arduino_uno::prelude::*;
const TURNING_TIME: u16 = 700u16;

/// The mutable wheels array is destructured for easier manipulation.
pub fn go_forward<>(wheels: &mut [arduino_uno::hal::port::Pin<arduino_uno::hal::port::mode::Output>; 4]) {
    let [left_forw, left_back, right_forw, right_back] = wheels;
    left_forw.set_high().void_unwrap();
    right_forw.set_high().void_unwrap();

    left_back.set_low().void_unwrap();
    right_back.set_low().void_unwrap();
}

pub fn go_backward(wheels: &mut [arduino_uno::hal::port::Pin<arduino_uno::hal::port::mode::Output>; 4]) {
    let [left_forw, left_back, right_forw, right_back] = wheels;

    left_forw.set_low().void_unwrap();
    right_forw.set_low().void_unwrap();

    left_back.set_high().void_unwrap();
    right_back.set_high().void_unwrap();
}


pub fn turn_right(wheels: &mut [arduino_uno::hal::port::Pin<arduino_uno::hal::port::mode::Output>; 4]) {
    stop(wheels);
    let [left_forw, _, _, _] = wheels;

    let mut delay = arduino_uno::Delay::new();
    left_forw.set_high().void_unwrap();
    delay.delay_ms(TURNING_TIME);

}
pub fn turn_left(wheels: &mut [arduino_uno::hal::port::Pin<arduino_uno::hal::port::mode::Output>; 4]) {
    stop(wheels);
    let [_, _, right_forw, _] = wheels;

    let mut delay = arduino_uno::Delay::new();
    right_forw.set_high().void_unwrap();
    delay.delay_ms(TURNING_TIME);

}

pub fn stop(wheels: &mut [arduino_uno::hal::port::Pin<arduino_uno::hal::port::mode::Output>; 4]) {
    let [left_forw, left_back, right_forw, right_back] = wheels;

    left_forw.set_low().void_unwrap();
    left_back.set_low().void_unwrap();
    right_forw.set_low().void_unwrap();
    right_back.set_low().void_unwrap();
}

SENSOR:

//! ### The sensor Module
//! The sensor module takes a SensorUnit value struct with a trigger, an echo
//! and a timer. As per HC-SR04 documentation, the trigger needs to be up 10 Ξs

use arduino_uno::prelude::*;
use arduino_uno::hal::port::mode::{Floating};

const TRIGGER_UP_TIME: u16 = 10u16;

/// struct sensor_unit is instantiated in main, as it needs
/// pins and timer.
pub struct SensorUnit {
    pub trig: arduino_uno::hal::port::portb::PB4<arduino_uno::hal::port::mode::Output>,
    pub echo: arduino_uno::hal::port::portb::PB3<arduino_uno::hal::port::mode::Input<Floating>>,
    pub timer: arduino_uno::atmega328p::TC1,
}

pub fn return_distance(sensor_unit: &mut SensorUnit) -> u16 {
    let mut delay = arduino_uno::Delay::new();
    // we are writing to the tcnt1 register:
    // https://docs.rs/avr-device/0.2.1/avr_device/atmega328p/tc1/tcnt1/type.W.html
    // when no special methods are listed, it is meant to use bits(), and
    // we can click on the W(rite) or R(ead) to see the implementation details.

    // Writing a value directly into a register is unsafe,
    // in case another register needs so you need to explicitely specify
    sensor_unit.timer.tcnt1.write(|w| unsafe { w.bits(0) });

    // void_unwrap() --> set high could return an error
    // we are using a crate named void + unwrap
    // if not, there will be a warning on the fact that result is not used
    sensor_unit.trig.set_high().void_unwrap();
    delay.delay_us(TRIGGER_UP_TIME);
    sensor_unit.trig.set_low().void_unwrap();

    'outer: while sensor_unit.echo.is_low().void_unwrap() {
        // if more than 200 ms ( = 50000) we might have not detected anything and can continue.
        if sensor_unit.timer.tcnt1.read().bits() >= 65000 {
            continue 'outer;
        }
    }

    // restarting the timer by writing 0 bits to it
    sensor_unit.timer.tcnt1.write(|w| unsafe { w.bits(0) });

    // waiting for the echo to get low again
    while sensor_unit.echo.is_high().void_unwrap() {}

    // Taking the time the echo was high, which is as long as the time the signal was out.
    // 1 timer count == 4 us so * 4 to get a value in microsecs
    // we divide by 58 to get the distance in cm, since (34000 cm/s * 1e-6 us time)/2 (back and forth measurement)
    // == 0.017 more or less 1/58
    let value = (sensor_unit.timer.tcnt1.read().bits() * 4) / 58;

    // !! AVR only natively supports 8 and 16 bit integers, so *do not* return bigger
    u16::from(value)

}

SERVO:

const SERVO_CENTER: u8 = 23;
const SERVO_RIGHT: u8 = 15;
const SERVO_LEFT: u8 = 31;

/// We use a generic for the pin
pub struct ServoUnit<S: embedded_hal::PwmPin<Duty=u8>> {
    pub servo: S,
}


/// We implement embedded_hal::PwmPin for the struct ServoUnit,
/// with rotations as methods and not lost functions
impl<S: embedded_hal::PwmPin<Duty=u8>> ServoUnit<S> {
    pub fn look_right(&mut self) {
        self.servo.set_duty(SERVO_RIGHT);

    }
    pub fn look_left(&mut self) {
        self.servo.set_duty(SERVO_LEFT);

    }
    pub fn look_front(&mut self) {
        self.servo.set_duty(SERVO_CENTER);

    }
}
2 Likes

Hi, I'm not an experienced Rustacean so I can't comment much on the Rustiness of your code.

The only thing that pops up to me that I don't like in your code is how you use your wheels pins. If you are going to use it like that, you should at least have an enum with the wheels indices there. Writing them by hand every time you use the wheels array is very error prone.

I'd also refactor the movement code out of the loop into its own module movement.rs or something. Maybe it's not necessary, but it would be nice to keep code compartmentalized as your project grows.

2 Likes

Thank you :slight_smile:
I am not sure I am 100% with you on that, do you mean it would be easier to use wheels[index] instead of destructuring?

Good point for the movement. I was thinking of eventually/maybe having a screen.

It's just that it's not hard to imagine someone screwing up the order of let [left_forw, left_back, right_forw, right_back] = wheels;.

On the other hand let left_forw = wheels[LEFT_FORWARD]; isn't easy to screw up, and if you do it's kind of easy to spot.

In any case, it's just my opinion :slight_smile:

1 Like

Just yesterday I was helping an intern at my company debug why his code was getting stuck somewhere, non-responsive.

He spent half an hour debugging because he wrote this:

for (int i = 0; buf->length; i++) {
   // stuff
}

Shit happens.

2 Likes

Think see what you mean.
In that case, I would need to add some constants and use them where destructuring.
Good point, it will allow more freedom in peoples wiring :slight_smile:

For example going forward needs 4 wheels:

pub fn go_forward<>(wheels: &mut [arduino_uno::hal::port::Pin<arduino_uno::hal::port::mode::Output>; 4]) {
    let [left_forw, left_back, right_forw, right_back] = wheels;

But not the turning functions:

pub fn turn_right(wheels: &mut [arduino_uno::hal::port::Pin<arduino_uno::hal::port::mode::Output>; 4]) {
    stop(wheels);
    let [left_forw, _, _, _] = wheels;

Ey this is super cool !!!
I might use a type alias for the Output pins like this

type OutputPin = arduino_uno::hal::port::Pin<arduino_uno::hal::port::mode::Output>;

and later in the functions arguments:

pub fn go_forward<>(wheels: &mut [OutputPin; 4])

Another improve could be implement a simple state machine for the logic

1 Like

Just saying, thanks for making this. I'll probably use it as a reference to get rust + uno working myself too

2 Likes

Thank you for your kind words and input :heart:

@elsuizo : I should definitely have more aliases, it would make it more readable.

@zireael9797 also updated the repo today to include information about how to get robot kits and how assemble them, it is the easiest way to order and make the electrical connections. I was absolutely overwhelmed the first time I needed to order electronic parts ... Maybe it is not the robot you had in mind but it might be interesting to know where you can order parts!

2 Likes

Yeah figuring out the hardware was pretty scary at first. Thankfully I've been doing this for a while and I likely have all the hardware I need to make a duplicate of your bot at home :blush: I might attempt to make either that or a remote control car with rust someday, though getting the bluetooth module to work might be pretty difficult.... so I might start with yours, or maybe a line follower.

1 Like

Yes please help yourself :slight_smile:!

I also forgot how confusing it is the first time to get hardware, especially now that I buy them on campus (we have a student club for electronics). Coincidence :slight_smile: I also want to have a bluetooth controlled robot! I will first do it on Arduino (and teach the kids I am mentoring), but then I want to rust it up!

Share your repo, I would love to follow your project and learn/ OR see your improvements it you take mine.

1 Like

Daja, good day, i wonder one question how you had successfully connected your program to this mechanic thing?) I want also create smth and confused how to correctly connect to smth i'll create,what you can suggest to me. I only heard about arduino, but this is smth childlike, i want smth don't know exactly, but to launch my mechanic )

I don't understand what you're trying to say but the best I can understand is you're making some kind of physical machine that has actual moving parts or other hardware, and you want a simple way to use code to make it run? If you want to do that an arduino really is the simplest way to go. You won't find anything more beginner friendly than an arduino uno.

1 Like

Okay, but i don't search the simplest way, i only want to connect and make it work as i say, interesting in smth that launch real mechanic in daily life)

That's not really the case. Arduinos are more like full-featured prototype boards that have most of the core electronics that a lot of projects will need. If I were going to develop a microcontroller-based product today, I'd probably get it working with an Arduino first. Only after I had a working prototype would I consider designing a custom board to reduce materials cost. Because the Arduinos are all open designs, I can copy the parts of the circuit I need into my own design later.

Can you tell us what you are trying to make?

Hi @Maxim

If you want to make the same robot as me, I recommend to buy this kind of kit:

And follow the instructions to assemble it:

and yes, start with Arduino, as @zireael9797 says you won't find friendlier .
The youtuber even provides Arduino code you can download.

To successfully connect the program (I assume you mean flash it?) you can use the Arduino IDE to upload it. Let us know what you are trying to build and we will try to help.

Thank you)

1 Like

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.