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

Pull request for easier build and support maple mini #60

Closed
vortex314 opened this issue Aug 22, 2022 · 5 comments
Closed

Pull request for easier build and support maple mini #60

vortex314 opened this issue Aug 22, 2022 · 5 comments

Comments

@vortex314
Copy link

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

  • use platformio and vscode to automate build without all pre-install requirements ( basically create a platformio.ini file and vscode plugin takes care of the rest )
  • support maple mini with auto-enumeration ( adapt pin's in device config )
  • https://stm32duinoforum.com/forum/wiki_subdomain/index_title_Maple_Mini.html
  • debug via vscode and platformio ( adapt launch.json for .vscode )
    However I have no experience in how to create a pull request or I don't have enough access to push a proposal branch.
    How do I proceed to push this code ?
    Regards
    Lieven
@r2axz
Copy link
Owner

r2axz commented Aug 22, 2022

Hi,

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

Best,
Kirill.

@vortex314
Copy link
Author

Hi,

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

Regards
Lieven

@r2axz
Copy link
Owner

r2axz commented Aug 26, 2022

@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
Copy link
Author

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.

  • .vscode/c_cpp_properties.json is an autogenerated file, there is no need to include it into the source tree; ==> I've included this so the code view doesn't deplay errors in visual code of not finding files. Could be indeed that in a proper auto-generation it's not needed

  • .vscode/launch.json contains absolute paths specific to your development environment, it cannot be included into the source tree; ==> I added this because it needs very specific option to be able to debug the code, not auto-generated

  • 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; ==> the coding style is enforced by the IDE. The maple dependencies can be found under the

#ifdef MAPLE
#else  
#endif      

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.
==> I didn't adapt the README yet as I was not able to make a pull request.

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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants