Multiple improvements for stm32wle targets #2309

ofauchon · 2021-11-23T23:43:10Z

This PR provide multiple improvements for STM32WLE5 targets:

board/stm32: Add STM32 Nucleo WL55JC board
board/lorae5,nucleo-wl55jc: Add SubGhz SPI3 interface
stm32/stm32wl: Set 48Mhz HSE32/PLL as default Clock, SubGhz Radio device init

kenbell

Thanks for improving the support - suggest just a small amount of replacing magic values with symbolics (where possible) - and needs some thought about how (and when) to initialize the radio peripheral.

Also - looks like a change may be missing based on the compile errors?

README.md

kenbell · 2021-11-24T15:33:40Z

src/runtime/runtime_stm32wle5.go

+func initCLK() {
+
+	// Set Power Voltage Regulator Range 2
+	//stm32.PWR.CR1.ReplaceBits(0b10, stm32.PWR_CR1_VOS_Msk, stm32.PWR_CR1_VOS_Pos)


Should either un-comment, or completely remove

kenbell · 2021-11-24T15:36:42Z

src/runtime/runtime_stm32wle5.go

+	}
+
+	// Set Flash Latency of 2 and wait until it's set properly
+	stm32.FLASH.ACR.ReplaceBits(0b010, 0b111, stm32.Flash_ACR_LATENCY_Pos)


Can use symbolic constants for these values?

All binaries replaced with symbolic constants.

kenbell · 2021-11-24T15:38:36Z

src/runtime/runtime_stm32wle5.go

+	}
+
+	// Configure PLL
+	stm32.RCC.PLLCFGR.Set(0x23020613)


Can use symbolic constants here? E.g. other STM32 worked on recently tend to have something like this:

// Configure the PLL stm32.RCC.PLLCFGR.ReplaceBits( (1)| // 1 = RCC_PLLSOURCE_MSI (PLL_M-1)<<stm32.RCC_PLLCFGR_PLLM_Pos| (PLL_N<<stm32.RCC_PLLCFGR_PLLN_Pos)| (((PLL_Q>>1)-1)<<stm32.RCC_PLLCFGR_PLLQ_Pos)| (((PLL_R>>1)-1)<<stm32.RCC_PLLCFGR_PLLR_Pos)| (PLL_P<<stm32.RCC_PLLCFGR_PLLPDIV_Pos), stm32.RCC_PLLCFGR_PLLSRC_Msk|stm32.RCC_PLLCFGR_PLLM_Msk| stm32.RCC_PLLCFGR_PLLN_Msk|stm32.RCC_PLLCFGR_PLLP_Msk| stm32.RCC_PLLCFGR_PLLR_Msk|stm32.RCC_PLLCFGR_PLLPDIV_Msk, 0)

I could rewrite the PLL init with symbolic constants . Done

kenbell · 2021-11-24T15:42:11Z

src/runtime/runtime_stm32wle5.go

+
+}
+
+// SubGhzInit() enables SX126x radio module


I don't think this should be in the runtime package - runtime is really about setting up clocks and time.

Should probably think about this as an optional peripheral that apps may (or may not) use - with it either:
1 - being in machine package
2 - being supported in the tinygo drivers repo

I'm not sure which is best - it's not a SPI/I2C peripheral - and might be a bit 'specialized' for the machine package? Perhaps @aykevl or @deadprogram have some thoughts?

SX126x Radio (which is part of STM32WL) will have its own tinygo driver soon, so the code can be reused when using SX126x standalone module.

But this function is specific to STM32WL series (as SX126x radio unit and MCU are on the same die)

IMHO it's entirely part of the whole chipset initialisation process.

I'm not sure moving this function to tinygo-driver makes sense (I don't see other situations you might need this)

But having it in machine package instead, would not hurt.

Other comments welcome .

machine package is a good place for this. Perhaps it should be exported so users who want to turn on this functionality can do so, but it is not always turned on by default?

SubGhzInit function moved in "src/machine/machine_stm32wle5.go" and exported so users can use it or not

deadprogram · 2021-11-24T21:16:30Z

Once you have updated from any changes from feedback, @ofauchon can you please rebase against the latest dev branch? Hopefully that will get CI to build successfully.

deadprogram · 2021-11-24T23:02:41Z

Something else must be wrong here @ofauchon see https://app.circleci.com/pipelines/github/tinygo-org/tinygo/6087/workflows/7a6938bf-d87d-4690-8782-209e919ae383/jobs/34339?invite=true#step-117-360

deadprogram · 2021-11-25T09:13:39Z

src/runtime/runtime_stm32wle5.go


 	// UART init
 	machine.Serial.Configure(machine.UARTConfig{})

 	// Timers init
 	initTickTimer(&machine.TIM1)
+
+	// Radio init
+	subGhzInit()


CI is now passing @ofauchon will you move subGhzInit() to machine package now, change to SubGhzInit(), and remove this call here so initialization is optional to the developer? 😸

ofauchon · 2021-11-25T10:35:39Z

@deadprogram @aykevl @kenbell .
I made all the suggested changes.

The final PR Code was tested on both LoraE5 and nucleo-wl55jc:

Simple timers tests OK ,
UART OK,
Radio module SX126x simple tests (Continuous TX checked with a SDR)

Olivier

aykevl

Generally looks good to me.
I'm a little bit unsure about the SubGhzInit function. IIRC it's on the same chip, but connected through an (on-chip) SPI bus?
I'm almost inclined to say it should be implemented entirely in a driver package, not in the machine package. Not sure. On the other hand, it does need some special initialization so it's not just any other SPI peripheral.

aykevl · 2021-11-25T14:11:03Z

src/machine/board_lorae5.go

+	// We assume a LED is connected on PB5
+	LED = PB5 // Default LED


It looks like this change is unrelated to the PR. Was it intentional?

This board don't actually have a LED.
So I thought I would be a good idea to remove it.

But, that breaks blinky1 , so I rolled-back the change.

FWIW in the LoRa-e5 schematic, PB5 is connected to a RED LED through a jumper (see bottom right of the schematic): https://files.seeedstudio.com/products/113990934/LoRa-E5%20Dev%20Board%20v1.0.pdf

It certainly blinked the LED for me :)

I understand why i've never seen this led blinking and why I connected an external LED (hopefully on PB5)

:-) the official dev board does look a little less artisanal.

aykevl · 2021-11-25T14:12:50Z

src/machine/machine_stm32wle5.go

+	switch spi.Bus {
+	case stm32.SPI1:
+		clock = CPUFrequency()
+	case stm32.SPI2, stm32.SPI3:
+		clock = CPUFrequency()
+	}


I don't understand this part. In all cases the clock equals the CPU frequency? Why not write clock := CPUFrequency() instead?

IIRC, this code was (quickly) borrowed from STM32F405

I guess the lines are the same because the both SPI1 and SPI2/SPI3 's bus use a 1:1 Prescaler.
(in the current STM32WL setup)

In the future, we may improve / tune the clocks, and the lines may turn into :

switch spi.Bus { case stm32.SPI1: clock = CPUFrequency() /2 case stm32.SPI2, stm32.SPI3: clock = CPUFrequency() /4 }

So we have two solutions:

Add comments to explain the PRESCALERs are 1:1 in the current configuration

Review and complete the work on SYSCLK/BUS/SPI clocks , prescalers... but that will take time

As I plan to work on Power Saving for this chip in a near future,
i'd prefer not spending too much time on the clocks right now, and juste add
some comment.

Tell me what you feel the most confortable with .

Olivier

Add comment about keeping separate to support future improvements, and IMO its all good.

I added the comment.
Thank you all for the time spend on this PR !

ofauchon · 2021-11-25T20:09:35Z

Generally looks good to me. I'm a little bit unsure about the SubGhzInit function. IIRC it's on the same chip, but connected through an (on-chip) SPI bus? I'm almost inclined to say it should be implemented entirely in a driver package, not in the machine package. Not sure. On the other hand, it does need some special initialization so it's not just any other SPI peripheral.

Yes, SPI3/SubGhz Is an internal SPI bus dedicated to SX126X Radio Submodule communication.

I agree this SubGhzInit is not ideal it looks like an ugly quirk in machine code.

I'll remove this function for the moment, and try to implement this in the tinygo-driver.

- board/stm32: Add STM32 Nucleo WL55JC board - stm32/stm32wl: Set 48Mhz HSE32/PLL as default Clock, SPI implementation

deadprogram · 2021-11-26T13:15:05Z

Thanks for responding to all the feedback @ofauchon

Now merging.

ofauchon mentioned this pull request Nov 23, 2021

board/stm32: Add STM32 Nucleo WL55JC board #2299

Closed

kenbell reviewed Nov 24, 2021

View reviewed changes

ofauchon force-pushed the stm32wl-pllclock-spi-subghz branch from fcc49e0 to b477afe Compare November 24, 2021 21:51

ofauchon force-pushed the stm32wl-pllclock-spi-subghz branch from b477afe to 629c5b6 Compare November 24, 2021 23:26

deadprogram reviewed Nov 25, 2021

View reviewed changes

ofauchon force-pushed the stm32wl-pllclock-spi-subghz branch 2 times, most recently from eebf58c to b4ec351 Compare November 25, 2021 10:19

ofauchon force-pushed the stm32wl-pllclock-spi-subghz branch 3 times, most recently from 66beb62 to 3928d7e Compare November 25, 2021 14:02

aykevl reviewed Nov 25, 2021

View reviewed changes

ofauchon force-pushed the stm32wl-pllclock-spi-subghz branch from 3928d7e to c045562 Compare November 25, 2021 19:22

Improvements for stm32wle targets :

9300598

- board/stm32: Add STM32 Nucleo WL55JC board - stm32/stm32wl: Set 48Mhz HSE32/PLL as default Clock, SPI implementation

ofauchon force-pushed the stm32wl-pllclock-spi-subghz branch from c045562 to 9300598 Compare November 25, 2021 20:21

ofauchon mentioned this pull request Nov 26, 2021

Add support for Generic Node Sensor Edition from The Things Industries #2066

Closed

deadprogram merged commit b0fce80 into tinygo-org:dev Nov 26, 2021

ofauchon deleted the stm32wl-pllclock-spi-subghz branch November 26, 2021 13:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple improvements for stm32wle targets #2309

Multiple improvements for stm32wle targets #2309

ofauchon commented Nov 23, 2021

kenbell left a comment •

edited

Loading

kenbell Nov 24, 2021

ofauchon Nov 25, 2021

kenbell Nov 24, 2021

ofauchon Nov 25, 2021

kenbell Nov 24, 2021

ofauchon Nov 25, 2021

kenbell Nov 24, 2021

ofauchon Nov 24, 2021

deadprogram Nov 24, 2021

ofauchon Nov 25, 2021

deadprogram commented Nov 24, 2021

deadprogram commented Nov 24, 2021

deadprogram Nov 25, 2021

ofauchon commented Nov 25, 2021

aykevl left a comment

aykevl Nov 25, 2021

ofauchon Nov 25, 2021

kenbell Nov 25, 2021

ofauchon Nov 26, 2021

kenbell Nov 26, 2021

aykevl Nov 25, 2021

ofauchon Nov 25, 2021

deadprogram Nov 25, 2021

ofauchon Nov 25, 2021

ofauchon commented Nov 25, 2021

deadprogram commented Nov 26, 2021

		// We assume a LED is connected on PB5
		LED = PB5 // Default LED

Multiple improvements for stm32wle targets #2309

Multiple improvements for stm32wle targets #2309

Conversation

ofauchon commented Nov 23, 2021

kenbell left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deadprogram commented Nov 24, 2021

deadprogram commented Nov 24, 2021

Choose a reason for hiding this comment

ofauchon commented Nov 25, 2021

aykevl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofauchon commented Nov 25, 2021

deadprogram commented Nov 26, 2021

kenbell left a comment •

edited

Loading