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

Updates to F0 and L0. #663

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@flabbergast

flabbergast commented Jun 1, 2016

This adds I2C support to STM32F0 - using the driver for F3, making it common for F0 and F3 (the I2C devices are sufficiently the same).

There are more changes/updates to STM32L0: add support for I2C (the same driver as for F0 and F3), for USART (the same driver as for F0), and some updates to rcc and flash (was incomplete).

I wanted to port the usb_dfu bootloader example to STM32L0 (it works now, sources here).

The L0 code was tested on my custom STM32L052C8T7 breakout board, the F0 I2C code was tested on my custom STM32F072CBT6 breakout (with a 24LC256 EEPROM).

@karlp

This comment has been minimized.

Member

karlp commented Jun 1, 2016

I'd rather see the i2c peripheral stuff you've named "f03" be named -v2 right away. Adding an f03 file and then in the same series using it for l0 seems pointless, and just highlights how bad an idea it was that the filenames would ever be meaningful long term.

*
* @ingroup STM32F0xx_defines
* @ingroup STM32L0xx_defines

This comment has been minimized.

@karlp

karlp Jun 1, 2016

Member

that can't be right.

This comment has been minimized.

@flabbergast

flabbergast Jun 1, 2016

Yep. Good catch.

@@ -23,6 +23,7 @@
/* --- STM32 specific peripheral definitions ------------------------------- */
/* Memory map for all busses */
#define FLASH_BASE (0x08000000U)

This comment has been minimized.

@karlp

karlp Jun 1, 2016

Member

does anything ever use this address? because flash is also at 0....

This comment has been minimized.

@flabbergast

flabbergast Jun 1, 2016

Right, nothing actually uses this. It just was there for F0 and F1, and some F1 code uses it.

RCC_ADC1 = _REG_BIT(0x34, 9),
RCC_ADC = _REG_BIT(0x34, 9),

This comment has been minimized.

@karlp

karlp Jun 1, 2016

Member

this was explicitly left out to discourage the adc/adc1 division making it harder to have code run on multiple targets. The intent was that everyone should just use adc1 even if they only had one adc.

This comment has been minimized.

@flabbergast

flabbergast Jun 1, 2016

OK, will remove. I just used the datasheet.

@@ -505,32 +507,40 @@ enum rcc_periph_clken {
RCC_TIM21 = _REG_BIT(0x34, 2),
RCC_TIM22 = _REG_BIT(0x34, 5),
RCC_MIFI = _REG_BIT(0x34, 7),
RCC_FW = _REG_BIT(0x34, 7),

This comment has been minimized.

@karlp

karlp Jun 1, 2016

Member

If this is meant to be the mifare firewall, then "MIFI" is the ref manual consistent name.

This comment has been minimized.

@flabbergast

flabbergast Jun 1, 2016

Just used stm32l0x2 ref man bit name (FWEN) (in RCC_APB2ENR). That ref man does not contain the word 'mifare', just uses 'firewall'.

This comment has been minimized.

@karlp

karlp Jun 1, 2016

Member

heh, then they've changed it in a newer rev :( My copy of rm0367 and rm0377 both call the bit MIFI In that case we should just make a single commit dropping MIFI and replacing it with FW.

@@ -539,6 +549,7 @@ enum rcc_periph_clken {
SCC_SRAM = _REG_BIT(0x40, 12),
SCC_CRC = _REG_BIT(0x40, 12),
SCC_TOUCH = _REG_BIT(0x40, 16),
SCC_TSC = _REG_BIT(0x40, 16),

This comment has been minimized.

@karlp

karlp Jun 1, 2016

Member

Caught :) yes, TSC is the ref man consistent name here. iirc TOUCH was already the name used by L1. open for debate here.

@@ -547,24 +558,30 @@ enum rcc_periph_clken {
SCC_TIM21 = _REG_BIT(0x44, 2),
SCC_TIM22 = _REG_BIT(0x44, 5),
SCC_ADC1 = _REG_BIT(0x44, 9),
SCC_ADC = _REG_BIT(0x44, 9),

This comment has been minimized.

@karlp

karlp Jun 1, 2016

Member

same as before

*
* @brief <b>Defined Constants and Types for the STM32F0xx USART</b>
*
* @ingroup STM32F0xx_defines

This comment has been minimized.

@karlp

karlp Jun 1, 2016

Member

nope, this is wrong.

This comment has been minimized.

@flabbergast

flabbergast Jun 1, 2016

Yep. Apparently I edited the wrong file when changing these :/

@flabbergast

This comment has been minimized.

flabbergast commented Jun 1, 2016

Yep. I wasn't sure about the naming conventions. I can rename the i2c business to v2 (makes more sense, since the same driver should work also on F7 and L4, but I don't have any of those so can't test).

What's the preferred way: edit history, or just fix and push to the same PR?

OBJS += usb.o usb_control.o usb_standard.o
OBJS += st_usbfs_core.o st_usbfs_v2.o
OBJS += usb.o usb_control.o usb_standard.o
OBJS += st_usbfs_core.o st_usbfs_v2.o

This comment has been minimized.

@karlp

karlp Jun 1, 2016

Member

While you've probably "fixed" things here, please avoid making unrelated whitespace changes in the same commit. It makes merges harder.

void rcc_css_hse_disable(void)
{
RCC_CR &= ~RCC_CR_CSSHSEON;

This comment has been minimized.

@karlp

karlp Jun 1, 2016

Member

extra tab?

*/
void rcc_clock_setup_in_hsi16_out_32mhz(void)
{

This comment has been minimized.

@karlp

karlp Jun 1, 2016

Member

nope. Please don't add more of this style. It's insane maintennance, and everyone wants to keep adding their own versions. I want to see everyone move towards the f4 style, and see also some notes on a old pull from esden on rcc overhaul. Adding methods like this has just been a disaster for maintaining consistent code.

This comment has been minimized.

@flabbergast

flabbergast Jun 1, 2016

I actually don't care for these either (hence just one). IMO these belong to examples, but the lib needs more examples for the 'more exotic' ones like L0 ;) I'm also not sure that they actually work across the whole family (e.g. L0 in this case).

Will remove this one (and add to my own code).

@karlp

This comment has been minimized.

Member

karlp commented Jun 1, 2016

This looks generally good, some long overdue cleanup of some parts! yay!

@flabbergast

This comment has been minimized.

flabbergast commented Jun 1, 2016

Sorry, the whitespace changes were not intended. BTW I couldn't figure out a/the pattern for OBJS in the Makefiles...

@karlp

This comment has been minimized.

Member

karlp commented Jun 1, 2016

for the file rename, rebase/amend and push -f to this PR, or a new PR. For the OBJS patterns, there wasn't really much of one. I'm trying to move more towards single lines for each object, space is cheap, and it avoids having to mangle lists when two people add objects. Had somewhat tried to be generally alphabetical and specific first, but nothing fixed, just "easy to merge"

@flabbergast

This comment has been minimized.

flabbergast commented Jun 1, 2016

Thanks for the comments! Will do changes in the evening.

The messy Makefiles' OBJS tickle my incipient OCD, but I suppose a sweeping change would break pretty much all the current PRs.

@flabbergast

This comment has been minimized.

flabbergast commented Jun 1, 2016

OK, did the proposed changes. Hopefully better now. For the deprecated names, I left them there with the note /* Compatibility */. I personally think it's OK to keep those to minimise breakage, but if the policy is to just remove them, I'll just remove them. I didn't touch/check F1 defs (may be in a similar situation).

About the RCC init functions: not present in the first 4 commits; the last one tries to do it for L0 in a similar way than F4.

/* USART_CR1 Values ---------------------------------------------------------*/
#define USART_CR1_M1 (1 << 28) /* F07x */

This comment has been minimized.

@karlp

karlp Jun 2, 2016

Member

this bit is on l0 too, so just drop this comment.

#define USART_CR1_OVER8 (1 << 15)
#define USART_CR1_CMIE (1 << 14)
#define USART_CR1_MME (1 << 13)
#define USART_CR1_M (1 << 12) /* Obsolete, please use M0 */

This comment has been minimized.

@karlp

karlp Jun 2, 2016

Member

current f0 refmans don't even have this bit anymore, I think we can decide it's been obsolete for long enough and just drop it. (and l0 refmans never had it as just "M")

#define USART_CR2_LBCL (1 << 8)
#define USART_CR2_LBIDE (1 << 6)
#define USART_CR2_LBDL (1 << 5)
#define USART_CR2_ADDM (1 << 4) /* Obsolete, use ADDM7 */

This comment has been minimized.

@karlp

karlp Jun 2, 2016

Member

same here, but these three little things we can just drop in a later commit I think. no need to update this pull for them.

RCC_GPIOH = _REG_BIT(0x2c, 7),
/* AHB peripherals */
RCC_DMA = _REG_BIT(0x30, 0),
RCC_MIF = _REG_BIT(0x30, 8),
RCC_CRC = _REG_BIT(0x30, 12),
RCC_TOUCH = _REG_BIT(0x30, 16),
RCC_TOUCH = _REG_BIT(0x30, 16), /* Compatibility */
RCC_TSC = _REG_BIT(0x30, 16),

This comment has been minimized.

@karlp

karlp Jun 2, 2016

Member

Relooking at it, there was no TOUCH used anywhere else, so this is just old names from an obsolete version of the ref man, and l0 is still super new, so I don't think we should feel at all bad about compatibility at this point. I would say just drop the _TOUCH entry, and say, "ST! you did a bad job with your early ref mans!"

RCC_TOUCH is currently only in l0, and only from teh rev1/rev2 of the refman. RCC_TSC is already existing for F3/F0/L4 and is the ref man style for l0 now. Let's just kill _TOUCH. (Same argument for _FW just below here, and the earlier define for RCC_AHBSMENR_TOUCHSMEN. Again though, we can probably just drop these in a standlone commit with that argument in it.

@karlp

This comment has been minimized.

Member

karlp commented Jun 2, 2016

first 4 look really good, rcc via structs looks pretty good too. Will try and get this in my staging queue next time I'm working on this in more detail than just reviewing diffs. Thanks for the great work!

@flabbergast

This comment has been minimized.

flabbergast commented Jun 4, 2016

Thanks again!
Just small touches, hopefully final: removed the F07x comment mentioned above, and updated the 'rcc via structs' commit to include also a setup function to use with HSE (I've soldered on a 16MHz crystal to my F052 board, and tested that it works). I can remove this if you don't want to add too many of these (although HSI16 and HSE make sense for me, these are the two most commonly used setups IMO).

@Salamandar

This comment has been minimized.

Salamandar commented Nov 29, 2016

Well that doesn't seem to work for me (or I'm missing something), on a stm32f031

    rcc_periph_clock_enable(RCC_GPIOB);
    rcc_periph_clock_enable(RCC_I2C1);
    // rcc_set_i2c_clock_hsi(I2C1);
    RCC_CFGR3 &= ~RCC_CFGR3_I2C1SW;

    gpio_mode_setup(
        GPIOB,
        GPIO_MODE_AF,
        GPIO_PUPD_PULLUP,
        GPIO6 | GPIO7);
    gpio_set_af(
        GPIOB,
        GPIO_AF1,
        GPIO6 | GPIO7);

    i2c_reset(I2C1);
    i2c_peripheral_disable  (I2C1);
    i2c_100khz_i2cclk8mhz   (I2C1);
    i2c_set_7bit_addr_mode  (I2C1);

    i2c_enable_analog_filter(I2C1);
    i2c_set_digital_filter  (I2C1, I2C_CR1_DNF_DISABLED);

    // i2c_enable_stretching   (I2C1);

    i2c_peripheral_enable   (I2C1);

    //      Init the MPU

    // Gyro soft reset
    // __asm("BKPT");
    to_send[0] = 128;

    i2c_send_data(I2C1, 100);
    while(1) {
        toggle_blue_leds();
        if (i2c_transfer_complete(I2C1))
            toggle_red_leds();
        delay_ms(500);
        // __asm("BKPT");
    }

Here the red leds don't blink as the blue leds do. And write_i2c is stuck in a while loop waiting for acquitement.
EDIT : The SDA and SCL gpios are maintained to 1 (I2C1 initializes the right GPIOs), so I don't have any clock or signal out.

(Sorry i'm posting this here, I couldn't do an issue on your repo @flabbergast.)

@SevenW

This comment has been minimized.

Contributor

SevenW commented Jan 19, 2017

What is the status of this pull request? I am looking for more extensive L052 support.

@karlp

This comment has been minimized.

Member

karlp commented Feb 9, 2017

This is close. I'm trying to get some decent i2c slave code (and spi slave code) so that I can do better testing of this i2c and your spi changes across a variety of platforms. I will probably just go find some standalone i2c eeprom to be the slave device and check this a bit. I've just not had any time for any testing or work on anything libopencm3 fo rthe last few weeks.

@Salamandar

This comment has been minimized.

Salamandar commented Feb 10, 2017

I have some Nucleo-L0, if you need some testing with a specific example I can try !

@karlp

This comment has been minimized.

Member

karlp commented Feb 10, 2017

The problem isn't really the boards, I've got that all pretty much covered, it's viable test targets and test software. ie, having a consistent "i2c slave" target that I can talk to, and consistent "i2c master" code I can then compile for f0, l0, f1, f3,f4,f7,l4 and test that they can all talk to the same i2c slave with (as much as possible) the same master side code. This is how regressions affecting the other platforms are caught, and how the api is kept being somewhat sane and portable across the project. PRs normally tend to include only the library side changes, and it's a rather slow process building up better test environments. (Writing a good spi slave/master pair and i2c slave/master pair is a bit of a priority right now)

@SevenW

This comment has been minimized.

Contributor

SevenW commented Feb 10, 2017

besides an i2c eeprom you could also consider a sensor. I tested with a BME280, which can be addresed through both the i2c and spi (from memory). I could share the L0 code that uses the sensor.

There are also various other sensors. I am now going to look into a Adafruit LIS3DH (can wake up a STM32L0xx from sleep while consuming below a uA.)

@karlp

This comment has been minimized.

Member

karlp commented Mar 2, 2017

I've taken the i2c changes from the other PR, they clean up the v1 parts too, but I'm pulling in the usart/flash/rcc changes from this PR now. whee

#define RCC_CFGR_PLLSRC_HSE_CLK 0x1
#define RCC_CFGR_PLLSRC (1<<16)
#define RCC_CFGR_PLLSRC_HSI16_CLK (0<<16)
#define RCC_CFGR_PLLSRC_HSE_CLK (1<<16)

This comment has been minimized.

@karlp

karlp Mar 6, 2017

Member

You're making this less like the L1. Why?

@karlp

This comment has been minimized.

Member

karlp commented Mar 6, 2017

I've pulled portions of this into my staging branch https://github.com/karlp/libopencm3/tree/karlp-staging-2017.1 , thanks for the work! The flash rework I'm afraid looks to be wrong, it has some very suspicious mangling of the PECR register bits, and adds busy waiting that wasn't in the original code. The RCC struct init for l0 is welcome, but it's too tightly tied with all the flash changes. The usart commonization code should have been added to the usart-v2 code, not a new file.

@karlp karlp closed this Mar 6, 2017

@kanflo

This comment has been minimized.

kanflo commented Jun 25, 2017

Has there been any work done on the L0 code in the past year? I just moved to L0s and noted support is missing on master. Glad to help anyway I can.

@karlp

This comment has been minimized.

Member

karlp commented Jun 26, 2017

@kanflo Please don't use closed tickets for questions like this. If you want to submit more l0 support, please consider the comments on the work here, and just generally try and submit good clear conciser commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment