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

[board] Nucleo-L496ZG-P #614

Merged
merged 6 commits into from
May 10, 2021
Merged

Conversation

rleh
Copy link
Member

@rleh rleh commented Apr 16, 2021

ST decided to use a different pinout for the Zio/Arduino connectors on this Nucleo-L144 board, see schematic.

The ST-Link serial lines are only connected to LpUart1 which is not yet supported by modm; thus the logger is disabled 😒.
I will have a look at the LpUart in the next days...

@rleh rleh force-pushed the feature/nucleo_l496zg branch from ab2fd1a to 2efdb8a Compare May 2, 2021 15:14
@rleh
Copy link
Member Author

rleh commented May 2, 2021

The LPUART driver is enabled now, but does not work yet on my Nucleo-L496...

@salkinium
Copy link
Member

salkinium commented May 2, 2021

Maybe the LPUART requires additional power bit enabled in the RCC? I remember some LP bits in there which are not used by the API yet.

@rleh
Copy link
Member Author

rleh commented May 2, 2021

I only see the LPUART1SMEN bit in RCC->APB1SMENR2, which is enabled by default and should not make any difference.

@salkinium
Copy link
Member

salkinium commented May 2, 2021

Ok, then it's maybe the clock source? RM says something about LSE being used. The LPUARTDIV prescaler is also 19-bits, instead of 16-bits.

Update: No, by default 00 LPUART1 seems to be clocked by PCLK like the other UARTS…

@salkinium
Copy link
Member

salkinium commented May 2, 2021

Yeah, baudrate computation has a different scalar of 256 for LPUART:

@rleh
Copy link
Member Author

rleh commented May 8, 2021

Issue: Uart is on GpioG7/GpioG8, but im unable to even toggle this GPIOs.

Then I found this little nasty detail:
grafik

Now I set this bit: PWR->CR2 |= PWR_CR2_IOSV;

But reading back PWR->CR2 always gives zero and gpio port G still does not work...
Any ideas?

@chris-durand
Copy link
Member

chris-durand commented May 8, 2021

Did you enable the PWREN bit in the RCC APB1ENR1 register? Otherwise the PWR peripheral is not clocked.

EDIT: looked at you branch, does not seem to be enabled.
So, do: RCC->APB1ENR1 |= RCC_APB1ENR1_PWREN

@rleh rleh force-pushed the feature/nucleo_l496zg branch from 2efdb8a to b4be026 Compare May 8, 2021 17:40
constexpr auto result = Prescaler::from_range(SystemClock::{{ uart_name ~ id }}, baudrate, 1, max);
%% elif uart_name == "Lpuart"
constexpr uint32_t max = (1ul << 19) - 1ul;
constexpr auto result = Prescaler::from_range(SystemClock::{{ uart_name ~ id }} * 256, baudrate / 6, 1, max);
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 have no explanation fot this magic /6 factor right now...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I was suspicious from the beginning, since the 19-bit - 16-bit = 3-bit, but the scalar is 256 = 8bit, so where do the other 5-bits go? But if this works, meh.

Copy link
Member

@chris-durand chris-durand May 8, 2021

Choose a reason for hiding this comment

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

I guess the /6 is not the correct solution, it is just a coincidence. The optimal prescaler for 115200 baud is 0x2b672 (177778), ST's table in the manual says 0x2b671 (177777) for 80 MHz clock. The code in this PR with /6 gives 171881. This looks pretty suspicious, still somehow close but not optimal.

Copy link
Member

Choose a reason for hiding this comment

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

This is an int overflow problem, we are calculating prescalers in 32 bit arithmetic.

>>> (80e6*256)/(2**31 - 1)
9.536743168503392

Copy link
Member

Choose a reason for hiding this comment

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

Oh crap, we should replace the modm::frequency_t and uint32_t with uint64_t in the modm::Prescaler class:
https://github.com/modm-io/modm/blob/develop/src/modm/math/algorithm/prescaler.hpp

Copy link
Member

Choose a reason for hiding this comment

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

We have to be careful, int overflow is evil. This compiles:

static constexpr uint32_t clock = 80'000'000;
static constexpr uint64_t clock2 = 80'000'000;

static_assert((clock * 256) != (clock2 * 256));

The system clock structs need an update as well.

Copy link
Member Author

@rleh rleh May 8, 2021

Choose a reason for hiding this comment

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

What do you think about my solution in adbe4da?

-class
-Prescaler
+template<typename T>
+class
+GenericPrescaler
{
// ...
// sed s/modm::frequency_t/T/
// ...
};

+using Prescaler = GenericPrescaler<modm::frequency_t>;

(except for the typo in the commit message)

Copy link
Member

Choose a reason for hiding this comment

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

It's good, it leaves the option open to use it at runtime with 32-bit, however I would just make it GenericPrescaler<uint64_t>; by default, since we only use it at compile time.

@rleh
Copy link
Member Author

rleh commented May 8, 2021

The LPUARTDIV prescaler is also 19-bits

Actually 20 bits 😉

Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Yesssssss (for everything but the prescaler issues)

@rleh rleh force-pushed the feature/nucleo_l496zg branch from b4be026 to a224093 Compare May 8, 2021 18:53
@rleh rleh force-pushed the feature/nucleo_l496zg branch from a224093 to 347acdd Compare May 9, 2021 01:39
@rleh rleh requested a review from salkinium May 9, 2021 01:39
@rleh
Copy link
Member Author

rleh commented May 9, 2021

Now also shared interrupts of any u(s)art should be supported.
Let's see what the CI thinks about it...

@rleh rleh added the ci:hal Triggers the exhaustive HAL compile CI jobs label May 9, 2021
@rleh rleh requested review from salkinium and chris-durand and removed request for salkinium May 9, 2021 13:31
@rleh rleh marked this pull request as ready for review May 9, 2021 13:32
@rleh
Copy link
Member Author

rleh commented May 9, 2021

Review, then squash and merge?

Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Very nice. Shared IRQs are kinda evil though… looks sternly at ST

@rleh rleh removed the ci:hal Triggers the exhaustive HAL compile CI jobs label May 9, 2021
@rleh rleh force-pushed the feature/nucleo_l496zg branch from 7f4cc65 to 897579e Compare May 9, 2021 23:57
@rleh rleh added the ci:hal Triggers the exhaustive HAL compile CI jobs label May 10, 2021
@salkinium salkinium merged commit 897579e into modm-io:develop May 10, 2021
@rleh rleh deleted the feature/nucleo_l496zg branch May 10, 2021 23:23
@salkinium salkinium added this to the 2021q2 milestone Jun 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:hal Triggers the exhaustive HAL compile CI jobs example 🔑
Development

Successfully merging this pull request may close these issues.

3 participants