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

SPI Implementation #13

Open
wants to merge 44 commits into
base: master
from
Open

SPI Implementation #13

wants to merge 44 commits into from

Conversation

@jonahbron
Copy link
Contributor

jonahbron commented Jul 3, 2019

In an attempt to execute #5 , I've begun on this skeleton of a SPI-implementing macro. I've only written a few lines of Rust, and never a macro, so this is based on reading the relevant documentation and following the examples of the other macros in the directory (namely serial.rs). I haven't even tried to compile it yet. It's a Work in Progress, and not yet ready to merge.

Please give any and all feedback. I will continue to work on fleshing out the details.

Note that I've intentionally substituted the original "master"/"slave" terminology for the alternate "primary"/"secondary" terminology. Hence "piso" and "posi" instead of "miso" and "mosi" respectively.

@Rahix

This comment has been minimized.

Copy link
Owner

Rahix commented Jul 3, 2019

Note that I've intentionally substituted the original "master"/"slave" terminology for the alternate "primary"/"secondary" terminology. Hence "piso" and "posi" instead of "miso" and "mosi" respectively.

Can you elaborate on why you did that?

@Rahix Rahix added the hal-impl label Jul 3, 2019
@jonahbron

This comment has been minimized.

Copy link
Contributor Author

jonahbron commented Jul 3, 2019

@Rahix Those terms carry a strong association with human slavery, usage of it can be insensitive to those who have been directly or indirectly impacted by that practice. See this section on Wikipedia on other projects that have migrated away from it and why. https://en.wikipedia.org/wiki/Master/slave_(technology)#Terminology_concerns

@jonahbron

This comment has been minimized.

Copy link
Contributor Author

jonahbron commented Jul 5, 2019

@Rahix The concrete implementation is "done". I still need to test it and turn it into a macro, but please review it to make sure it seems sane at this point. Please offer any desired critique of the interface too.

@jonahbron

This comment has been minimized.

Copy link
Contributor Author

jonahbron commented Jul 6, 2019

Pretty happy with the current state of the concrete implementation. Need to test it next, then make a macro out of it.

@jonahbron

This comment has been minimized.

Copy link
Contributor Author

jonahbron commented Jul 8, 2019

Having trouble testing this implementation. I've been trying to verify it at a simple level by creating an example that connects POSI directly to PISO to read and write the same byte. However it's not working, it just reads a 0. Continuing debugging.

@jonahbron

This comment has been minimized.

Copy link
Contributor Author

jonahbron commented Jul 8, 2019

I've simplified my example to cut out the SPI implementation entirely and use the registers directly, but it still does not work as expected.

#![no_std]
#![no_main]
#![feature(proc_macro_hygiene)]

extern crate panic_halt;
use arduino_uno::prelude::*;

#[no_mangle]
pub extern fn main() -> ! {
    let dp = arduino_uno::Peripherals::take().unwrap();

    let mut delay = arduino_uno::Delay::new();
    let mut pins = arduino_uno::Pins::new(
        dp.PORTB,
        dp.PORTC,
        dp.PORTD,
    );
    let peripheral = dp.SPI;
    let mut secondary_select = pins.d10.into_output(&mut pins.ddr);

    let mut serial = arduino_uno::Serial::new(
        dp.USART0,
        pins.d0,
        pins.d1.into_output(&mut pins.ddr),
        57600,
    );

    peripheral.spcr.write(|w| {
        w.spie().clear_bit();// disable SPI interrupt
        w.spe().set_bit();// enable SPI
        w.dord().clear_bit();// set up data order control bit
        w.mstr().set_bit();// set to primary mode
        w.cpol().clear_bit();// set up polarity control bit
        w.cpha().set_bit();// set up phase control bit
        w.spr().val_0x03()// set up clock rate control bit
    });
    peripheral.spsr.write(|w| w.spi2x().set_bit());// set up 2x clock rate status bit
    loop {
        ufmt::uwriteln!(&mut serial, "config: {}", peripheral.spcr.read().bits()).unwrap();
        secondary_select.set_low().unwrap();
        peripheral.spdr.write(|w| w.bits(0b10101010));
        while peripheral.spsr.read().spif().bit_is_clear() {}
        ufmt::uwriteln!(&mut serial, "response: {}\r", peripheral.spdr.read().bits()).unwrap();
        secondary_select.set_high().unwrap();
        delay.delay_ms(1000);
    }
}

Some output

config: 87
          waiting
response: 0
config: 87
          waiting
response: 0
config: 87
          waiting
response: 0
config: 87
          waiting
response: 0
config: 87
          waiting
response: 255
config: 87
          waiting
response: 0
config: 87
          waiting
response: 0
config: 87
          waiting
response: 0
config: 87
          waiting
response: 0

Oddly sometimes the byte will come back as 0b11111111. No idea why. I've tweaked a bunch of the control register settings and even checked that the config (87) matches what I expect according to the datasheet.

At this point I'm wondering if this is even possible (connecting PISO to POSI). I'll probably try integrating with the Ethernet module I have next. I wanted to wait as long as possible on that, since it's a less primitive test and will be more difficult to debug.

@jonahbron

This comment has been minimized.

Copy link
Contributor Author

jonahbron commented Jul 9, 2019

I did make an attempt at getting the Ethernet module to work, but it wouldn't even initialize before panicking. According to this StackExchange answer though, my idea of looping PISO to POSI should work.

https://arduino.stackexchange.com/questions/66942/spi-test-by-connecting-mosi-to-miso/66950#66950

I'll continue debugging with that simple setup.

@jonahbron

This comment has been minimized.

Copy link
Contributor Author

jonahbron commented Jul 11, 2019

Finally had an epiphany and solved the problem. The problem was that the POSI pin was not in output mode. Should have the SPI library working and ready soon now. Here's a super basic (working) example:

#![no_std]
#![no_main]
#![feature(proc_macro_hygiene)]
extern crate panic_halt;
use arduino_uno::prelude::*;
#[no_mangle]
pub extern fn main() -> ! {
    let dp = arduino_uno::Peripherals::take().unwrap();
    let mut delay = arduino_uno::Delay::new();
    let mut pins = arduino_uno::Pins::new(
        dp.PORTB,
        dp.PORTC,
        dp.PORTD,
    );
    pins.d10.into_output(&mut pins.ddr);// POSI pin must be made an ouptput
    pins.d11.into_output(&mut pins.ddr);// secondary select pin must be made an output
    let mut serial = arduino_uno::Serial::new(
        dp.USART0,
        pins.d0,
        pins.d1.into_output(&mut pins.ddr),
        57600,
    );
    dp.SPI.spcr.write(|w| {
        w.spie().clear_bit();
        w.spe().set_bit();// must enable SPI
        w.dord().clear_bit();
        w.mstr().set_bit();// must set to primary mode
        w.cpol().clear_bit();
        w.cpha().clear_bit();
        w.spr().val_0x00()
    });
    dp.SPI.spsr.write(|w| w.spi2x().clear_bit());

    loop {
        dp.SPI.spdr.write(|w| w.bits(0b10101010));
        while dp.SPI.spsr.read().spif().bit_is_clear() {}
        let read_data = dp.SPI.spdr.read().bits();

        ufmt::uwriteln!(&mut serial, "data: {}\r", read_data).unwrap();
        delay.delay_ms(1000);
    }
}
@jonahbron

This comment has been minimized.

Copy link
Contributor Author

jonahbron commented Jul 12, 2019

Successfully moved the working SPI code into the atmega328p library and added an example that uses it. Next up I need to add more docs to the example, then move it back into the macro.

@jonahbron

This comment has been minimized.

Copy link
Contributor Author

jonahbron commented Jul 12, 2019

Documentation is ready, all that's left is turning it into a macro.

@jonahbron

This comment has been minimized.

Copy link
Contributor Author

jonahbron commented Jul 12, 2019

@Rahix Please see the comment I left above.

@Rahix

This comment has been minimized.

Copy link
Owner

Rahix commented Oct 23, 2019

I will review this PR in the following days. Sorry for keeping you waiting for this long!

Copy link
Owner

Rahix left a comment

Hey @jonahbron,

sorry for keeping you waiting this long ... (4 months now??) Thanks for your work on this, I hope I'll have a chance to try it against some real peripheral hardware soon!

I took a look at and ran some tests with your branch, I have left a few comments on the code. There are two more points which are a bit more general:

  • I'd prefer moving the macro-instanciations into each chip-crates lib.rs instead of a separate file (eg. chips/atmega32u4-hal/src/spi.rschips/atmega32u4-hal/src/lib.rs). I have done the same thing for I2C and serial (see commit 335d5ef ("Move usart into lib.rs")), with the intention of keeping things closer together and not having a large number of files with very few lines of code.
  • While I totally agree with your reasons behind switching to the primary/secondary naming scheme, I don't think this is appropriate here. For any new protocol design I would definitely support going away from the old, badly chosen terms but SPI is a standard that is clearly defined using those terms and I have never seen anyone use anything else. I don't think the confusion we'd cause with this is worth it and they only pop up in very few places throughout your PR anyway.

During testing, I noticed that one weird behavior I mentioned in another comment. I am not sure whether this is an SPI specific thing or just another compiler weirdness. If I can find some more time, I will investigate further. Could you try it on your hardware and see whether you get the same behavior?

avr-hal-generic/src/spi.rs Outdated Show resolved Hide resolved
avr-hal-generic/src/spi.rs Outdated Show resolved Hide resolved
avr-hal-generic/src/spi.rs Outdated Show resolved Hide resolved
boards/arduino-leonardo/src/pins.rs Outdated Show resolved Hide resolved
boards/arduino-leonardo/src/pins.rs Outdated Show resolved Hide resolved
chips/atmega328p-hal/Cargo.toml Outdated Show resolved Hide resolved
avr-hal-generic/src/spi.rs Outdated Show resolved Hide resolved
avr-hal-generic/src/spi.rs Outdated Show resolved Hide resolved
boards/arduino-uno/examples/uno-spi-feedback.rs Outdated Show resolved Hide resolved
@jonahd-google

This comment has been minimized.

Copy link

jonahd-google commented Jan 18, 2020

Hi @Rahix . I too am sorry for leaving this for 4 months now 😂 . I just moved states and got a new job. I'm finally ready to pick this back up. Note that I'm on a different account now. My new employer declined to give a copyright waiver on this project, so any work I do is under their copyright instead of mine; thus I'm doing it under a dedicated work account.

Thanks for much for the detailed feedback. I'll start going through and applying it.

@Rahix

This comment has been minimized.

Copy link
Owner

Rahix commented Jan 18, 2020

Hey @jonahd-google,

I too am sorry for leaving this for 4 months now. I just moved states and got a new job. I'm finally ready to pick this back up.

No problem, I left you hanging for way longer :p Hope you are doing great in your new place!

Note that I'm on a different account now. My new employer declined to give a copyright waiver on this project, so any work I do is under their copyright instead of mine; thus I'm doing it under a dedicated work account.

Alright I guess. I am always a bit wary with corporate involvement ... Would it be ok for you to sign the Developer Certificate of Origin for your work on this project?

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.
@jonahd-google

This comment has been minimized.

Copy link

jonahd-google commented Jan 20, 2020

Alright I guess. I am always a bit wary with corporate involvement ... Would it be ok for you to sign the Developer Certificate of Origin for your work on this project?

Totally understand. I was really disappointed they wouldn't allow me to do it independently; they cited Fuchsia as the reason. However, I'm also not allowed to sign any CLAs without approval. I'll email a request for review and see what they say. If they don't approve, I get it if you want to apply the feedback yourself and merge my existing (pre-Google) contribution.

@jonahd-google

This comment has been minimized.

Copy link

jonahd-google commented Jan 22, 2020

@Rahix I got approval on that DCO. I should have the updated PR ready for you next week.

@jonahbron

This comment has been minimized.

Copy link
Contributor Author

jonahbron commented Feb 3, 2020

I've applied all of your locally. I have to go through a little bit of a process to push up the changes the first time, but hopefully that process can go through this week.

@jonahd-google

This comment has been minimized.

Copy link

jonahd-google commented Feb 3, 2020

Patch pending an internal process. You can check here to see my full pending patch in the mean-time.

jonahd-google/avr-hal@0aef1a6...bcc913f

Specifically the bits I changed to address your review:

jonahd-google/avr-hal@5233da4...bcc913f

jonahbron and others added 3 commits Feb 3, 2020
Applied feedback requested by Rahix
Removed unused SpiError enum
@jonahd-google

This comment has been minimized.

Copy link

jonahd-google commented Feb 3, 2020

Somehow I didn't quite catch all of the feedback you left. Will need to take another pass at finishing up.

@jonahbron

This comment has been minimized.

Copy link
Contributor Author

jonahbron commented Feb 6, 2020

@Rahix I've addressed all of your feedback. Please go over everything and let me know if there's anything you don't feel I've sufficiently corrected. I'm excited to finally have this upstream 😄

@Rahix
Rahix approved these changes Feb 11, 2020
Copy link
Owner

Rahix left a comment

Looks good! I am very busy this week but I will hopefully have some time to test your PR next weekend :) Thanks again for your efforts!

@Rahix

This comment has been minimized.

Copy link
Owner

Rahix commented Feb 16, 2020

Still fighting the compiler to even compile anything for AVR again ... Did you need to fix anything to get your toolchain working?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.