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

Add UART driver for SAMG #761

Merged
merged 1 commit into from
Oct 22, 2021
Merged

Conversation

mcbridejc
Copy link
Contributor

No description provided.

src/modm/platform/uart/samg/uart.cpp.in Outdated Show resolved Hide resolved
src/modm/platform/uart/samg/module.lb Outdated Show resolved Hide resolved
@salkinium salkinium added this to the 2021q4 milestone Oct 22, 2021
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, thanks!

@salkinium salkinium added the ci:hal Triggers the exhaustive HAL compile CI jobs label Oct 22, 2021
@salkinium
Copy link
Member

Hm?

 x   samg55g19a-uu          37.4s
In file included from modm/src/modm/platform.hpp:47,
                 from main.cpp:12:
modm/src/modm/platform/uart/uart_7.hpp: In static member function 'static void modm::platform::Uart7::initialize(modm::platform::UartBase::Parity, modm::platform::UartBase::WordLength, uint8_t)':
modm/src/modm/platform/uart/uart_7.hpp:69:3: error: 'USART7' was not declared in this scope; did you mean 'USART2'?
   69 |   USART7->US_BRGR = result.index;
      |   ^~~~~~
      |   USART2

@mcbridejc
Copy link
Contributor Author

Hm?

 x   samg55g19a-uu          37.4s
In file included from modm/src/modm/platform.hpp:47,
                 from main.cpp:12:
modm/src/modm/platform/uart/uart_7.hpp: In static member function 'static void modm::platform::Uart7::initialize(modm::platform::UartBase::Parity, modm::platform::UartBase::WordLength, uint8_t)':
modm/src/modm/platform/uart/uart_7.hpp:69:3: error: 'USART7' was not declared in this scope; did you mean 'USART2'?
   69 |   USART7->US_BRGR = result.index;
      |   ^~~~~~
      |   USART2

image

Well, it seems USART7 is only available on the 'j' parts, not the 'g' parts. I guess I need to add some filtering based on the device-pin field in module.lb.

@mcbridejc
Copy link
Contributor Author

Your tests are good :)

@salkinium
Copy link
Member

Well, it seems USART7 is only available on the 'j' parts, not the 'g' parts. I guess I need to add some filtering based on the device-pin field in module.lb.

That's weird, since modm-devices should apply this filter automatically? This smells like a bug in modm-devices, I'll check that out.

@mcbridejc
Copy link
Contributor Author

mcbridejc commented Oct 22, 2021

That's weird, since modm-devices should apply this filter automatically? This smells like a bug in modm-devices, I'll check that out.

I think I get it. The flexcom driver doesn't have the device-pin field, and I'm using device.has_driver("flexcom:samg*") to find the USARTs. For some reason, the samg55g19 CMSIS header includes FLEXCOM7, but not USART7/SPI7/TWI7. I'm guessing this is a mistake.

@mcbridejc
Copy link
Contributor Author

I changed to device.has_driver("usart:samg*"), and I think that will fix it.

@salkinium
Copy link
Member

Ah, I see. This isn't a bug in modm-devices, but perhaps a bug in the data?
FLEXCOM always has instance 7, but TWI, SPI and USART only have it on J devices?
Can this be correct?

    <driver name="flexcom" type="samg55">
      <instance value="0"/>
      <instance value="1"/>
      <instance value="2"/>
      <instance value="3"/>
      <instance value="4"/>
      <instance value="5"/>
      <instance value="6"/>
      <instance value="7"/>
    </driver>

@salkinium salkinium added ci:hal Triggers the exhaustive HAL compile CI jobs and removed ci:hal Triggers the exhaustive HAL compile CI jobs labels Oct 22, 2021
@mcbridejc
Copy link
Contributor Author

mcbridejc commented Oct 22, 2021

Ah, I see. This isn't a bug in modm-devices, but perhaps a bug in the data? FLEXCOM always has instance 7, but TWI, SPI and USART only have it on J devices? Can this be correct?

No I don't think so. I don't think it makes sense. I suspect it's just a mistake in the vendor data.

@salkinium salkinium merged commit 6e9f000 into modm-io:develop Oct 22, 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 🔑 feature 🚧
Development

Successfully merging this pull request may close these issues.

2 participants