Initial support for SPI and I2C

[dkm]

This is an early (but working) support for both SPI and I2C. I2C has been tested with a DS 3231 RTC + logical analyzer. SPI has been tested with MAX7219 + logical analyzer.

Chances are that there are still some bugs, but it's still better than no support at all :)

I'm still new to Rust, so there may be some construct that are not idiomatic... In particular, the types used for alternate function are still a bit new for me, so my solution may not be the best.

[dkm]

As a side note, this has been tested with ek-lm4f120xl and ek-tm4c123gxl boards.

[thejpster]

Many thanks for this PR - I'm really excited about adding I2C and SPI support for this board. I see what you mean about the type state for I2C. I'm going to have a bit of a think about how best to ensure the I2C GPIO pins are set in the correct mode and get back to you. I'm not sure giving AF3 a mode in itself is right, but I don't know what the right answer is either.

[dkm]

Ok. Maybe I could try to do something at the same level as the AFx types, ie. AF3OD ? Do not hesitate to give even hint of potential things to try; I'm doing this as a way to learn rust, so any exercise is good for me :)

[dkm]

I can also split this PR in 2 (spi & i2c) so they can be merged separately.

[thejpster]

OK, I've had a bit of a think, and I'd like to propose a alternative solution to the extra type state this adds to AF3. If the I2C driver takes a GPIO pin which implements SdaPin then we know the I2C driver "owns" that pin - no one else can use it. We also know it must already have been placed into the correct Alternate Function mode (which may vary depending on the pin and the peripheral, but in this case seems to be AF3 for I2C), but we do not know whether the Open Drain bit has been set. As there is almost never a scenario where you don't want your SDA pin in Open Drain mode, I think the best thing to do is to add an unsafe fn for all GPIO pins, regardless of which mode they are in, which sets the Open Drain bit. The I2C driver should then call this unsafe function on the SDA pin it is provided, to ensure the Open Drain bit is set.

[dkm]

Makes sense :) I can give it a try if it's ok for you !

[dkm]

Small question: why is it not the global model (ie. letting drivers own the abstract pin and configure it as they want) ?

[thejpster]

why is it not the global model

I'm afraid I don't follow. The model this library follows is the one set out by stm32f30x-hal and the output of svd2rust as described in http://blog.japaric.io/brave-new-io/.

[dkm]

Sorry for the delay,busy week.... I can see why the low level GPIO config is not pushed in drivers code when it's not the driver's work to decide if it needs to set the pctl to value x or y depending on which device it is setting up. But what if for a given driver, the config is always the same ? In our case, i2c pins always have pclt == 3, and 1 pin must have open drain set. I don't see it as contradictory to the stm reference, but maybe I'm wrong.

I find it odd to have the pin setup split between the caller and the callee. If the previous proposition is still not ok by you, wouldn't it be a bit more homogeneous to have an extra AF3OD type (along with AF1, AF2, AF3, ...) ? There would be an extra into_af3od() that returns the pin in the correct state (same as in my current code, without the extra type generics).

[thejpster]

So the concept of using the GPIO module to set pin config and passing pre-configured pins into the peripheral was a model proposed by @japaric and one I'm keen to follow to get some consistency.

I've proposed a new GPIO API for handling Alternate Functions in https://github.com/thejpster/tm4c123x-hal/tree/new_gpio_af. It allows for AF pins to be Open Drain or Push Pull, and if in Open Drain lets you specify floating or pull-ups (I figured pull-down was useless on an Open Drain pin). Let me know what you think.

[dkm]

Hi ! As said before I'm new so it's still a bit hard to compare different solutions. But as you said, it looks more consistent. When you merge these changes in your master branch, I'll rebase my commit ! Thanks !

[thejpster]

New GPIO stuff merged.

[dkm]

I've rebased all my changes and was able to test that SPI is still working. I need to do some basic tests for I2C and then I can submit my changes for review.

Thanks !

[dkm]

Ok, I've tested both I2C and SPI and they seem to work. Can't say that I've been stress-testing them very hard, but at least with both my devices (max7219 and DS3231) on both boards (tm4c and lm4f), it works.

Do not hesitate to make me change part of the code... I'm still learning...

[thejpster]

Thank you for taking the time to submit this PR. Sorry, I've been tied up with election things recently. I'll make a note to check on this shortly.

[dkm]

Tell me when you think it's best for me to refresh this PR. I see that you've done some cleanup, including in spi/i2c. I can rebase it if you think you can review it :)

[thejpster]

Please do rebase, I should have a chance to take a look before RustFest.

[dkm]

May 19, 2018 8:39 PM, "thejpster" notifications@github.com wrote:

Please do rebase, I should have a chance to take a look before RustFest.

Rebased and pushed ! Let me know if you want me to clean/change anything. I don't have the board to test the result of the rebase, but my test code builds.

Have a nice RustFest next week ! Hope to be able to attend a next edition :)

[dkm]

Rebase tested on hardware, works as expected...

[thejpster]

This looks great! Just a couple of comments in-line about the SPI calculations.

[dkm]

You want me to add more comments about how to compute SPI values, or do you want me to remove some of them ?

[jonathanpallant]

Ah, I meant I've left comments in-line in the source code on the "Files changed" page.

[jonathanpallant]

Oh, but now I can't see them. Humm. Anyway, I was concerned regarding the selection of CSPR and the downcast to a u8. Once you've got a u8, it'll definitely be <= 255, so wasn't sure the assert was useful, or it was doing the right thing if a candidate cspr was > 255.

[dkm]

Ah ! You are right, but then it seems I won't correctly handle cases where there is no solution. I'll rework this part to make sure I get all corner cases ! Thanks for the review !

[dkm]

Ok, I've pushed a fixed version of my branch. I'm still not sure what should be done in case of error. Looking at STm32 crates does not show any example of error handling in constructors...

[jonathanpallant]

I can't see any changes. I've got src/spi.rs from c26bbab. Is that right?

[dkm]

Yes, that's correct. In this version I compute the cpsr value using u16 and still checking that we find something using assert, and then I downcast the value to u8 before its real usage.

[jonathanpallant]

scratch head

I'm looking at

[dkm]

Yes, but cpsr is declared as u16 before the loop and shadowed by a u8 declaration after. But you're right that scr has the same problem, I was too quick and only focused on cpsr, sorry.

[dkm]

I've reworked this loop, hope it is correct now. I've rebased over your latest changes and also updated the copyright notice. It still works correctly with my test setup.

[dkm]

:)

[thejpster]

Yeah, sorry, snowed under atm.

I'm still confused though - Github tells me commits were made on May 25, 2018, but that the individual commits are from 27 March. Have you been adding new commits with changes arising from the comments above, or re-writing the old commits?

[dkm]

No problem, was simply pinging :) I've been rebasing the first commits without refreshing the creation date. All changes discussed above have been merged in the initial commits.

[thejpster]

Oh, that's why my in-line comments have been disappearing then and the dates are all confused.

Without those comments to check against, I guess this is fine to merge.

[dkm]

Thanks !