r2axz / bluepill-serial-monster

USB to 3 Port Serial (UART) adapter firmware for STM32 Blue Pill.
MIT License
323 stars 76 forks source link

Pull request for easier build and support maple mini #60

Closed vortex314 closed 2 years ago

vortex314 commented 2 years ago

Hi, I cloned the repo locally and did slight modifications on the device config to enable some more features :

r2axz commented 2 years ago

Hi,

please send me the link to your branch with these modification. Is it available on GitHub?

Best, Kirill.

vortex314 commented 2 years ago

Hi,

the branch can be found here : https://github.com/vortex314/bluepill-serial-monster

Regards Lieven

r2axz commented 2 years ago

@vortex314 while the overall idea of adding maple support seems reasonable, I have a number of questions regarding the implementation:

  1. .vscode/c_cpp_properties.json is an autogenerated file, there is no need to include it into the source tree;
  2. .vscode/launch.json contains absolute paths specific to your development environment, it cannot be included into the source tree;
  3. device_config.c was heavily modified and coding style was significantly changed. It is hard to see maple-support related changes hidden in coding style changes. Anyway, if you want to add anything into the code, you should follow my coding style, not enforce yours;
  4. Same applies to usb_io.c, however I don't see any non-formatting related changes there at all;

In the device_config.c the comment / pin is occupied by USB / is wrong. PIN is occupied by the LED. Even if the PIN is occupied by the LED, we should think about remapping it (are there free PINs), not disabling. Also I don't see a single word about Maple in README.

At this point your branch cannot be merged.

Question: what exactly should be changed for maple support? Is it only about the LED pin?

vortex314 commented 2 years ago

Hi Kyrill, thanks for the detailed review, I admire the project you dis so far, some answers :

Overall I used the platformio setup as it is much more convenient to get all pre-requisites than the steps described in the README. Also from my own experience I know the USB renumeration on a bluepill is unpredicatble, I had better results with maple boards.

At this point your branch cannot be merged.

Question: what exactly should be changed for maple support? Is it only about the LED pin? ==> It's about the LED PIN but also the hidden pin in the usb_io.c code to drive the USB reset

#ifdef MAPLE
    GPIOB->CRH &= ~GPIO_CRH_CNF9;
    GPIOB->CRH |= GPIO_CRH_MODE9_1;
#else
    GPIOA->CRH &= ~GPIO_CRH_CNF12;
    GPIOA->CRH |= GPIO_CRH_MODE12_1;
#endif
    for (int i = 0; i < 0xFFFF; i++)
    {
        __NOP();
    }
#ifdef MAPLE
    GPIOB->CRH &= ~GPIO_CRH_CNF9;
    GPIOB->CRH |= GPIO_CRH_MODE9_1;
#else
    GPIOA->CRH &= ~GPIO_CRH_CNF12;
    GPIOA->CRH |= GPIO_CRH_MODE12_1;
#endif

Hope this helps, Lieven

vortex314 commented 2 years ago

I tried not to configure the txa pin for UART2 , however it needs one as otherwise it crashes in

__attribute__((always_inline)) inline static void usb_cdc_usart_irq_handler(int port, USART_TypeDef * usart,
    volatile uint32_t *txa_bitband_clear) {
    uint32_t wait_rxne = 0;
    uint32_t status = usart->SR;
    if (status & USART_SR_TC) {
        *txa_bitband_clear = 1;  // ==> memory fault 
        usart->CR1 &= ~(USART_CR1_TCIE);
    }

As the code & doc in my repo https://github.com/vortex314/maple-serial-monster is incomplete for all cases,forget about the pull request. Regards