Skip to content

WIP: Support for autogenerated mcu cores #4

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

Merged
merged 38 commits into from
Nov 5, 2018

Conversation

dylanmckay
Copy link
Member

@dylanmckay dylanmckay commented Aug 30, 2017

It should be possible to do something like this

extern crate arduino;
use arduino::{Pin, HardwareSpi};
use arduino::cores::atmega328p;

trait Netlist {
    type PushButton: Pin;
    type Led: Pin;
    type Spi: HardwareSpi;
}

struct ATmega328p;

impl Netlist for ATmega328p {
    type PushButton = atmega328p::PORTB3;
    type Led = atmega328p::PORTB4;
    type Spi = atmega328p::SPI;
}

fn start<N: Netlist>() {
    N::PushButton::set_input();
    N::Led::set_output();
    N::Spi::setup_master();

   loop {
        // delay
        M::Led::set_high();
        // delay
        M::Led::set_low();
    }
}

This would allow users to easily mix-and-match supported microcontrollers, provided the microcontroller has enough GPIOs, etc.

src/spi.rs Outdated
/// http://maxembedded.com/2013/11/the-spi-of-the-avr/
pub trait HardwareSpi {
/// Master-in slave-out pin.
type MISO: Pin;
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if this is a losing battle, but I really was hoping we could avoid having acronym soup. I'm happy that there are comments, but I would rather it be

type MasterInSlaveOut: Pin;

And now the comment is encoded in the code. We can then add comments describing the why and the how, instead of the what.

The reason I think it might be a losing battle is because there's going to be inertia from all sides to "just do what Arduino / C / Atmel does". The reason I don't think that's fully valid is because we want to do things as Rust-like as we can.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is a good idea.

src/spi.rs Outdated
}

/// Constants for the control register.
#[allow(dead_code)]
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? Should it be public?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to expose everything just yet because the design isn't 100%, will be public eventually though

src/pin.rs Outdated

/// An IO pin.
pub trait Pin {
/// The associated data direction registerr.
Copy link
Member

Choose a reason for hiding this comment

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

extra r

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

src/reg.rs Outdated
@@ -0,0 +1,95 @@
use core::{cmp, convert, ops};

pub trait RegVal : Copy + Clone +
Copy link
Member

Choose a reason for hiding this comment

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

Is there are reason RegVal needs to be abbreviated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

build.rs Outdated

fn generate_cores(mcus: &[Mcu]) -> Result<(), io::Error> {
for mcu in mcus {
generate_core_module(mcu).expect("failed to generate mcu core");
Copy link
Member

Choose a reason for hiding this comment

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

This returns a result; do we just want to propagate?

Copy link
Member

Choose a reason for hiding this comment

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

Or perhaps not until we have an error type to keep the extra information from the expect text

@pusherofbrooms
Copy link

There's some great stuff in this PR. Is there any help needed to get it across the finish line? I'd like to use some of the features in https://github.com/pusherofbrooms/rust-arduino-examples without using the extra special branch.
I like that this PR eliminates the std boiler plate.

@dylanmckay
Copy link
Member Author

Is there any help needed to get it across the finish line?

At the moment, it adds autogenerated core functionality side-by-side with the current arduino master. It's a bit strange having two different implementations of the library, and so I think that if we can get feature parity with current master with the new implementation, that should be good.

IIRC, I did most of the work to get it feature complete - I think the SPI stuff is still to be done?

@dylanmckay
Copy link
Member Author

Alright, this is now feature complete with the current master branch as far as I can tell.

Still need to do some cleanups, and the whole serial module is still using the old atmega328-specific constant tables (rather than the autogenerated core stuff). The serial module can stay that way for now though

@dylanmckay
Copy link
Member Author

This is ready for some more eyes again @shepmaster @pusherofbrooms @Restioson

@dylanmckay
Copy link
Member Author

I'm going to merge this now to stop faffing about. It's much easier for people to build utop what is here when it is in master, rather than my private fork. I've looked over it locally, cleaned it up a lot. It should work on https://docs.rs too, automatically assuming atmega328p.

@dylanmckay dylanmckay merged commit 7a8572e into avr-rust:master Nov 5, 2018
@dylanmckay
Copy link
Member Author

Docs on docs.rs.

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.

3 participants