search

Rahix / avr-hal

embedded-hal abstractions for AVR microcontrollers
Apache License 2.0
1.32k stars 223 forks source link

avr-hal-generic Usart is insufficiently generic #201

Open quentinmit opened 3 years ago

quentinmit commented 3 years ago

I've been trying to implement a Usart type for ATtiny167 and have been running into typing hell trying to do that.

The avr-hal-generic crate currently contains the Baudrate struct that computes the values of the UBRR and U2X registers directly. However, ATtiny doesn't use those registers, it uses LINBRR and LINBTR. I could really use some guidance on how to move Baudrate into the atmega-hal crate and make the UsartOps struct in avr-hal-generic properly generic. Also, the Event enum is likewise atmega-specific.

I think I have an implementation of Baudrate/Uart for ATtiny that should work as soon as I can figure out how to properly parameterize the types.

quentinmit commented 3 years ago

Also, hot take: the generic module should be called uart and not usart. Yes, the mega peripheral is a USART, but the interface the module exposes is purely asynchronous. The tiny peripheral is only a UART (actually it's a combined LIN/UART peripheral) so it's weird to implement UsartOps for it...

Rahix commented 3 years ago

The avr-hal-generic crate currently contains the Baudrate struct that computes the values of the UBRR and U2X registers directly. However, ATtiny doesn't use those registers, it uses LINBRR and LINBTR.

As discussed OOB, I agree that Baudrate should become a part of the specific HAL now. I wonder how exactly we should implement this, though, as currently we rely on the BaudrateExt and BaudrateArduinoExt traits to fix a board-specific errata. Maybe those can be moved over easily as well? - or, maybe those belong into arduino-hal instead, anyway?

Rahix commented 3 years ago

Also, hot take: the generic module should be called uart and not usart.

You do have a point. If you are willing to send a PR to change it, I'd accept :) Although I am unsure where to draw the line - for example, should the module in atmega-hal also be renamed? Or kept the same, because the peripherals are actually USART?

quentinmit commented 3 years ago

Also, hot take: the generic module should be called uart and not usart.

You do have a point. If you are willing to send a PR to change it, I'd accept :) Although I am unsure where to draw the line - for example, should the module in atmega-hal also be renamed? Or kept the same, because the peripherals are actually USART?

Hm, I'd say the type should definitely still be called Uart if it's only implementing asynchronous serial. But the module, well, where would you put a type for synchronous serial? hal::usart::SynchronousSerial? I could see an argument that drivers for a peripheral should all live in the same module.

Rahix commented 3 years ago

Hmm, not entirely sure how I feel about it but what about hal::serial::Uart and hal::serial::Usrt or something along those lines?

quentinmit commented 3 years ago

I like hal::serial::Uart. Not sure about hal::serial::Usrt... but you can postpone that until you actually have a high-level API for synchronous serial. (I guess you could have two types called Uart and Usart and then have traits called hal::serial::Asynchronous and hal::serial::Synchronous, the latter of which is only implemented on Usart?)

wt commented 3 years ago

From what I see, it seems that it also is not generic enough to implement the USB CDC interfaces on boards like the leonardo. I would love to help get this implemented.

Rahix commented 3 years ago

@wt, USB CDC-ACM is completely separate from UART/USART which is what this issue is about. See #40 for that.

G33KatWork commented 2 years ago

I am currently trying to add support for the ATtiny{4,8,16,32}17s. I found the ATmega4809 pull request which I am basing my code on. So far I have my hardware running and the only abstraction present is the GPIO pins like already implemented for the mega4809. I changed the atxmega HAL a bit to align it with the other hals so that I have the pins! macro and can access other unsupported peripherals using their register definitions in the PAC. So far, so good - apart from finding an LLVM bug that affects all XMEGA cores.

I just wanted to add basic support for the UART, but I ran into the problem that the baud rate is calculated differently. The new pre-dividers are 8 and 16, depending on the double clock mode or not, the old ones were 4 and 8, so I can't use the current Baudrate implementation.

Before I just add these dividers as const-generics and somehow build a macro which set the right dividers for the right chips, is there a "proper" way to abstract this and maybe also support the LINBRR/LINBTR-chips and other future chips that might work differently? What would that look like? Any suggestions?

Also, another thing that I don't know how to handle yet: The PORTMUX. The newer XMEGA cores allow multiplexing the peripheral pins to two or sometimes even more different sets of GPIOs. They can only be multiplexed as whole groups, so it's not like with the STM32s where you can multiplex single pins arbitrarily. Here you need to multiplex all UART/SPI/TWI-pins of the affected peripheral at the same time. Right now the pins are tied to the UART as generics. How would we be able to handle multiplexing them? See the ATtiny817 datasheet, page 19.