STMicroelectronics / stm32-mw-usbpd-core

USB Power Delivery core stack library component for STM32
Other
5 stars 3 forks source link

Get_Battery_Status Extended Message isn't functioning #1

Open davedesro opened 1 year ago

davedesro commented 1 year ago

Overview

The library does not allow the developer to set / control the value of Battery Status Ref when calling Get_Battery_Status extended message

Details

When calling

USBPD_StatusTypeDef ret;
uint8_t data = 0; 
ret = USBPD_PE_SendExtendedMessage(0, USBPD_SOPTYPE_SOP, USBPD_EXT_GET_BATTERY_STATUS, &data, 1);
HAL_Delay(300); /**<-- for simplicity, to keep stack data valid */

The library returns USBPD_OK, but what gets sent on the CC line is an invalid Get_Battery_Status message. Size == 0 in the extended header.

When I set uint8_t data = 1, then Size = 1 in the extended header, which is better.. however, the Battery Status Ref is set to 0x40, which is invalid.

Info

Screenshots

These were taken from Total Phase's USB PD sniffer + their DataCenter software

  1. data = 0. Note the "Size" area in pink is 0, but should be '1' dataIsSetTo0

  2. data = 1. Note the "Size" area is '1', but the Battery Status Ref (incorrectly named to Battery Cap Ref), is incorrectly set to 0x40, not '1'. See "Data" in Green dataIsSetTo1

This is a critical piece for our application. Please let me know if you'd like any more information.

HFISTM commented 1 year ago

Hello,

Thank you for this report. We will get back to you as soon as we analyze it further. This may take some time. Thank you for your comprehension.

With regards,

HFISTM commented 1 year ago

ST Internal Reference: 156169

davedesro commented 1 year ago

Hi @HFISTM thank you.

I have an update: the Total Phase PD Sniffer itself is reporting bad data (there's a bug in the PD Sniffer data parsing).

Still, for Battery_Status command, the library is not giving any notification that one was received (I confirmed at the HAL that the return data is valid)

Also, when the port-partner returns Battery_Status command, the library always issues a SOFT_RESET back to the port-partner.

So, in summary, the issue as I see things now are 1) Upon Battery_Status inbound command, there is no Battery_Status notification (No call to USBPD_DPM_SetDataInfo()) 2) Upon Battery_Status inbound command, SOFT_RESET is always issued

davedesro commented 1 year ago

Hi @HFISTM Update. This issue is seen with a Samsung S20.

I get a tag of 15 (USBPD_CORE_BATTERY_STATUS) in USBPD_DPM_SetDataInfo() for an Anker power brick.

davedesro commented 1 year ago

@HFISTM one more observation: when in USBPD_DPM_SetDataInfo(), if it copy the Battery_Status data in Ptr-2, it seems to work. (see image.. "Ptr-2") Is it possible the library is incorrectly looking at +2 byte offset of the Battery_Status response?

image

HFISTM commented 1 year ago

Hello @davedesro , to be sure to understand, what is your Datatype_BatteryStatusInfo field ? SetDataInfo(USBPD_CORE_BATTERY_STATUS) is supposed to write Ptr direclty (no offset) to DPM_BatteryStatus field, of type USBPD_BSDO_TypeDef which is defined in the PD spec. Pleaase refer to this application as exemple.

Also could you confirm your issue still present on latest library (v5.0.0) ? I noticed you were on v4.2.0. Thanks

davedesro commented 1 year ago

Hi @HFISTM I updated to 5.0.0 and see the same issue... I need to subtract by 2 (Ptr-2).

Thank you for the link to the application. I believe there may be an issue with the code.

image

https://github.com/STMicroelectronics/STM32CubeG0/blob/master/Projects/STM32G081B-EVAL/Demonstrations/DemoUCPD/Src/usbpd_dpm_user.c#L856

The code is copying only 1 byte (Ptr[0]) to a uint32_t. Shouldn't it copy 4 bytes? This is incorrect, but it might also hide the Ptr-2 issue

davedesro commented 1 year ago

@HFISTM @GCASTM There may be another issue with 5.0.0,

When the library is in UFP+SRC mode, the functions USBPD_PE_Request_CtrlMessage() USBPD_PE_SendExtendedMessage() do not work. SVDM responses still work, however.

All other combinations of UFP/DFP + SNK/SRC work fine. I'm able to call ctrl messages.

Shall I create another ticket for this issue, since it seems like a regression?

In the meantime, I will continue to use v4.2.0

HFISTM commented 1 year ago

Hello @davedesro, Thank you for your feedback. For any new issue, please open another ticket with details.

Regarding the battery status, you are right about the copy of only 1 byte instead of 4. Could you please try with the following code ?

  case USBPD_CORE_BATTERY_STATUS:
    {
      uint8_t* battery_status;
      battery_status = (uint8_t*)&DPM_Ports[PortNum].DPM_BatteryStatus.d32;
      memcpy(battery_status, Ptr, Size);
    }

This might also fix the "-2" issue, as the previous code was only taking the first byte of the message, which was not the actual BatteryInfo response. Please refer to struct USBPD_BSDO_TypeDef: batteryInfo field is the 3rd of the 4 bytes in this struct. As you were only copying the first byte, substracting 2 to the pointer directly copied the 3rd byte, meaning the BatteryInfo you were looking for.

With this fix you should be able to access the BatteryInfo field through the USBPD_BSDO_TypeDef struct directly as it will now be fully copied by SetDataInfo function.

Thank you & regards