search

ardnew / STUSB4500

Arduino library for USB PD sink controller (STUSB4500)
MIT License
29 stars 3 forks source link

Portable C Implementation #5

Open oclyke opened 1 month ago

oclyke commented 1 month ago

Hello there. Thank you for authoring this library. It seems to be the most complete (as compared with the SparkFun lib) and simple (as compared with others like the ST reference software or an Entree USB-PD CAN adapter board I found on Tindie.)

I would like to see the dependency on Arduino's facilities abstracted away so that the logic for updating NVM, managing profiles, etc can be used on other devices.

The advantage, as I see it, is to gain more community support around a single repository and enable users to easily port the functionality to their own target. Users could depend on this repo as a git submodule, add the necessary (core) source files and headers into their own build system, and provide their own interface definitions.

My proposed approach would be as follows:

As a maintainer you would not be expected to support any additional platforms as long as the Arduino reference implementation continues to work through this abstraction.

You can see an example of how I have implemented this structure in the past with SparkFun's ICM-20948 library. Here's a link to the portable C header file: https://github.com/sparkfun/SparkFun_ICM-20948_ArduinoLibrary/blob/main/src/util/ICM_20948_C.h

I will start work on this project in my own fork of your repo (thank you for MIT licensing!) and if you're interested in bringing it all under one roof I would love to see that happen.

Thanks for your consideration!

ardnew commented 1 month ago

It's pretty painful to look back on this code today :), but a couple of comments or suggestions:

  1. I don't believe this library supports the integrated NVM functionality of this IC (unverified; don't remember). It wasn't a feature I cared to add, because there were numerous other libraries that already provided that.
    • The value of this library is that it can reconfigure the PD profiles in real-time over I²C and transition between profiles without power loss.
    • AFAIK, no other STUSB4500 library is capable of real-time configuration[^1]. They all rely on the NVM approach, i.e., "write configuration to NVM, disconnect, renegotiate". This is much simpler to implement, but it seems sorta hacky to me and results in power loss every time you select another profile. This is particularly an issue if your device managing the STUSB4500 is itself powered via USB VBUS.
  2. Yes, please gut the Wire library and any dependency on Arduino altogether. There is no reason for them. The I²C interface is a no-brainer abstraction[^2], and a handful of platform examples would cover the majority of use-cases so that most users wouldn't have to provide a concrete implementation anyway.
  3. I would personally prefer a modern C++ port, but if accessibility is the goal I think C is the right choice. Though, don't overlook my Go port! This port is much better designed, and it probably does a better job at abstracting the serial communications. The monitor is a state machine abstraction at a higher-level than the PD protocol's, and the example demonstrates general use.
  4. Ideally, the library structure/API would be redesigned. I would use this existing code simply as a reference implementation for correctly processing alerts/interrupts (void STUSB4500::processAlerts(void)) and the PD state machine (void STUSB4500::update(void)). These are the trickiest, most nuanced parts of the code. They may look straightforward, but the order and timing is quite delicate to support bus/signal requirements of both I²C and USB PD simultaneously. Again, refer to the Go port for comparison.

Hope all this helps! Had a lot of fun with this IC.

[^1]: The official ST driver package does provide an example of real-time moderation, but it is heavily dependent on STM32 HAL and can only target a single host device (NUCLEO-F072RB). This Arduino library was originally a "generalized" (hah) port of that ST firmware.

[^2]: The following C++ is an example I²C interface I use in my projects. It's the bare minimum so that it's easy to implement everywhere, and it's generalized enough to cover most use cases:

Click to expand I²C interface source code (C++) > ```c++ > #pragma once > > #include > #include > > // Enclose the abstract class in namespace "proto" to prevent name clashes, > // and to make it clear that it must be implemented — not instantiated directly. > namespace proto { > > // Pure abstract class that defines the interface for communicating register > // read/write operations over I²C. > // > // The I²C protocol itself does not define any concept of memory or registers. > // Conventionally, these are implemented using multi-message transactions where: > // - the first message identifies the address or command; and > // - subsequent messages read or write the data at that address. > // > // Using multi-message transactions introduces another problem: the byte order > // of message data is unspecified. In general, there is not a conventional byte > // order, so it must be specified per device. > // > // This class does not use fixed-width buffers for data transfer, which gives > // derived classes full control over memory allocation. However, this requires > // derived classes to also handle all byte order conversions. > struct i2c { > // (Re)Initialize the I²C controller interface. > // > // The I²C hardware and I/O pins must already be initialized. > virtual bool init(const std::uint8_t, const std::uint32_t) = 0; > > // Write data with the given number of bytes to the specified memory address, > // and return the number of bytes successfully written. > virtual std::size_t write(const std::uint8_t, const void *, const std::size_t) = 0; > > // Read the given number of bytes from the specified memory address, and > // return the number of bytes successfully read. > virtual std::size_t read(const std::uint8_t, void *, const std::size_t) = 0; > }; > > } // namespace proto > ```