manuelbl / usb-pd-arduino

USB Power Delivery for Arduino
MIT License
57 stars 10 forks source link

Add G4 family support #7

Closed VIPQualityPost closed 9 months ago

VIPQualityPost commented 10 months ago

Add support for other G4 Nucleo boards (G431RB, G474RE, G491RE) Add support for building "standard" G4 family (G474xx, G491xx) Use ARDUINO_NUCLEO defines to clarify pins used on dev kits for CC pins. Closes #6.

VIPQualityPost commented 10 months ago

I didn't add all details for G491xx, because I don't have hardware to test, but I checked and it should not have any pin differences from the G474xx configuration. I'm not sure whether to leave out entirely or just add complete support, or something in between so that people can try it.

manuelbl commented 10 months ago

Thanks for the PR. I have an additional Nucleo G4 family board coming my way and will test it later this week.

VIPQualityPost commented 9 months ago

Thanks! I have some G0 boards that I will also try to add some support for those as well. I am still working to understand the PD protocol a bit better. I would eventually like to add support for SRC role. Can I ask how you discovered the issue with SOP markers earlier? You cannot debug this with print statements, right? Do you just follow along in the debugger and have a strong understanding of the protocol?

manuelbl commented 9 months ago

I used PlatformIO and ran the test/VoltageChange project in debug mode.

Since the timing is critical, it's not really possible to stop at a breakpoint and continue. The power source will have given up by that time. It's more of a one-shot debugging. If a breakpoint is hit, you can examine the variable values and continue for a few steps to see what branches are taken. But then you have to set different breakpoints and start over.

Using that technique, I tried to understand why it wouldn't set a voltage different from 5V: the break point (the function for setting it) was never hit. That way it went backwards to the event dispatch, GoodCRC transmitting until I finally discovered that the log had been saying "INV" instead of "SOP" all the time. From there is was easy.

manuelbl commented 9 months ago

The Nucleo boards have arrived: a Nucleo G474RE, a Nucleo G071RB and the SNK1M1 shield. I've extended the library to cover the latter two as well, in addition to the G474RE you have already provided.

The code is still a bit too Nucelo specific. I need to go over the #ifs again and make it more generic such that it works for generic boards with the same MCUs as well.

VIPQualityPost commented 9 months ago

There is available the ARDUINO_GENERIC_G**** define, but then this requires needing both Nucleo and Generic defines.

Are you sure that actually it's required to set the pin with pinMode before doing the GPIO configuration with LL driver (as this is the only code which really requires the knowledge of whether it's a Nucleo or not)? I haven't checked but I don't think the framework needs to use this, in which case we can just use STM32G4xx / specific chip defines defines for configuring CC lines, and remove the pinMode stuff entirely.

manuelbl commented 9 months ago

The Arduino pinMode configuration has been implemented because I've studied the Arduino STM32 core code and concluded that it would be the safest solution. If I remember correctly, I was afraid that the Arduino core would disable the GPIO clock if certain other pins were reconfigured using the Arduino functions. By initializing them with pinMode, it would know that one of the pins is still in use and that the clock can't be stopped.

I have to re-read the code and check if it is still required. It would indeed be simpler if it wasn't needed.

VIPQualityPost commented 9 months ago

I was thinking - usually when I am working with stm32duino I just use the "real" names e.g. PB4 rather than "Arduino-fied" names. And it seems that the framework is doing this anyway when you use any Arduino name, it will call something like

void ArduinoPinToHAL(uint32_t APin, uint32_t* HALPin, GPIO_TypeDef** HALPort)
{
    uint32_t Pin = digitalPinToPinName(APin);
    *HALPort = get_GPIO_Port(STM_PORT(Pin));
    *HALPin  = STM_LL_GPIO_PIN(Pin);
}

anyway, so I think maybe we can just skip this and directly use pinMode(PB4, INPUT_ANALOG) rather than "D10" or specific numbers that depend on layout but are unchanged on the silicon.

manuelbl commented 9 months ago

If that works, it would be very helpful. It would basically cover entire STM32 families.

VIPQualityPost commented 9 months ago

I was going to make the changes today, but discovered you already addressed it! Thanks! Here is something I built with your library, doing motor control over single wire (USB power + data): https://youtu.be/Vx6fxW4_j_4

manuelbl commented 9 months ago

That looks great! Thanks for sharing.