Features/rs485

[Rishabg24]

This PR Implements lib/RS485 the integration of the RS485 transceivers for the Toad Engine Controller. The code allows for the baud rates to be dynamically switched as needed, and uses the hardware's core to mux the RS485s on the UART Busses, allowing for single-digit microsecond switching between sending and receiving.

[RobertJN64]

Broadly I think we should trade against three options - want @jacobh460's input.

1. Code as is - with additional documentation for the register fiddling we are doing, particular in the DE function.
2. Use HAL_RS485Ex_Init
3. Use existing .begin() function, then use the internals from HAL_RS485Ex_Init:

  /* Disable the Peripheral */
  __HAL_UART_DISABLE(huart);

  /* Perform advanced settings configuration */
  /* For some items, configuration requires to be done prior TE and RE bits are set */
  if (huart->AdvancedInit.AdvFeatureInit != UART_ADVFEATURE_NO_INIT)
  {
    UART_AdvFeatureConfig(huart);
  }

  /* Set the UART Communication parameters */
  if (UART_SetConfig(huart) == HAL_ERROR)
  {
    return HAL_ERROR;
  }

  /* Enable the Driver Enable mode by setting the DEM bit in the CR3 register */
  SET_BIT(huart->Instance->CR3, USART_CR3_DEM);

  /* Set the Driver Enable polarity */
  MODIFY_REG(huart->Instance->CR3, USART_CR3_DEP, Polarity);

  /* Set the Driver Enable assertion and deassertion times */
  temp = (AssertionTime << UART_CR1_DEAT_ADDRESS_LSB_POS);
  temp |= (DeassertionTime << UART_CR1_DEDT_ADDRESS_LSB_POS);
  MODIFY_REG(huart->Instance->CR1, (USART_CR1_DEDT | USART_CR1_DEAT), temp);

  /* Enable the Peripheral */
  __HAL_UART_ENABLE(huart);

  /* TEACK and/or REACK to check before moving huart->gState and huart->RxState to Ready */
  return (UART_CheckIdleState(huart));

I would lean towards option 3 with a comment pointing back to this HAL function because it lets us re-use more existing code, but not sure, and there might be a fourth clever way to do this that I'm missing.

[RobertJN64]

I would consider something like:

class RS485Uart : Uart {
RS485Uart(tx, rx, de) : uart_(tx, nc, nc, nc), de_(de) {}

begin() {
uart_.begin()
// disable
// reg flips to enable de
// enable
}
}

[RobertJN64]

I'm fine to merge before testing this, but IMO #2 priority on EC bringup (after core bringup like basic UART printing) is probing the DE pin and confirming that this logic works as expected.

[jacobh460]

Here are my comments:

• DE pin on MCU side is not being muxed to the UART (need to configure it!)
  Check out __HAL_RCC_GPIOx_CLK_ENABLE(), HAL_GPIO_Init()
  Use these in the RS485Bus::begin() routine since this only needs to be done once

(note: we should probably enable a pull up resistor on the UART_RX pin so that the line doesn't float when there is no mux selected!)

• re the comment in RS485.h: "NOTE: DO NOT call uart.begin() on a bus after RS485s::begin() since it will re-enable RTSE and kill DE."

  • can you show where this enables RSTE? from what I can see, stm32duino Uart doesn't touch RTS/CTS flow control unless you explicitly pass pins in the constructor (defaults to NC) and doesn't set RTSE/CTSE in the configuration registers unless RTS and CTS are not NC
  • good point about this killing DE

• You don't clear RTSE in CR3 (in RS485Bus::applyDE), though CTSE gets cleared?

In setBaud(), you are calling a HAL function instead of using Arduino's Uart::begin() (which can be called repeatedly to change the baud rate). Probably best to use the arduino function to change the baud rate so there isn't a mismatch between what Arduino thinks the baud rate is currently set to and what it is actually set to. Then do minimal work to enable DE as well.

• I like Robert's option 3

• can you justify the critical section in RS485Bus::setBaud()? if not, maybe instead check that there is no active transfer, then disable the peripheral and proceed from there rather than wrapping in a critical section. critical sections are best avoided when not necessary.

[RobertJN64]

• can you show where this enables RSTE? from what I can see, stm32duino Uart doesn't touch RTS/CTS flow control unless you explicitly pass pins in the constructor (defaults to NC) and doesn't set RTSE/CTSE in the configuration registers unless RTS and CTS are not NC

Previously I was passing the DE pin into the RTS pin param of the constructor - if we fix like in option 3 above that won't be an issue.

[jacobh460]

Ok got it disregard what I said about gpio init, was wrong about that. Just make sure the alternate function matches

[RobertJN64]

return false in these

[Rishabg24]

Done. My only concern is that here if we return false immediately, then in the case that both bus6 and bus2 fail, we don't know the latter fails until we fix bus6. Whereas if we have a flag, and let both conditionals evaluate then return false, we will know if both busses or just one of them is broken at the same time.

[RobertJN64]

Ah - I would be fine with a flag as well - issue was that previously we weren't failing on bus2 status at all.

[RobertJN64]

LGTM - let's merge after testing on Saturday!

[RobertJN64]

LGTM