Refactoring proposals/questions

[puzrin]

Hi mate! Thank you for outstanding efforts for creating oss pd stack.

I'm interested in PD Sinc PPS stuff. It would be nice to make this project more "flexible" for easy use in outer projects as solid library. Unfortunately, low-level debugging is not my strong side, but I could help with API design & do some preliminary rewrites (at least, to a "buildable" state).

Here are my assorted thoughts about the current state of things:

• No possibility of using external phy drivers (could be passed to the constructor as param).
• The same about a timer.
• The same about arduino.h dependencies.
• It may be worth moving drivers to a subfolder.
  • In the "ideal world" drivers should be separate packages. However, I understand the difficulties of such code management at earlier stages. So, these are ok here, but would be nice to separate files location and API more strictly.
• Lack of fusb302 driver
  • with a possibility to use different i2c methods (bitbang & dma)
  • with additional arbiter level (i2c locks) to allow top project share i2c for multiple devices
• It may be worth using ETL for generic things (instead of custom ring buffer and so on)
• The concept of "delayed threads" looks suspicious mixture of FSM and threads concepts. Probably, it is worth adding more states or moving delays into thread-like code.
  • I'm not 100% sure if protothreads will help here. In the "ideal world" code should be a pure FSM with a separate state for each async operation (including delays) OR a small set of protothreads with multiple waits for state changes inside (timers, semaphores, messages). Still have not dug too deep into current state transitions to be certain about protothreads usability here.

Let me know what you think.

PS. Closing this to not annoy you with "endless issues". If you are interested and could share your thoughts, I will prepare something more concrete for next iterations.

[manuelbl]

Thanks for your input.

I agree with several of them, in particular making peripherals like PHY and timers more pluggable and composable. But it's not without it challenges:

• On STM32 chips, peripherals and their use of pins can often be mutually exclusive. And the Arduino core also grabs several one. So it would involve quite an effort to figure out valid combinations and test them.
• Even if peripherals are pluggable, they still require a lot of ifdefs as they won't compile on all platforms.

A FUSB302 driver shouldn't be too difficult. I've used this chip before.

I'm less convinced about using ETL. Arduino has no library dependency management. So pulling in a library just for a simple ring buffer is probably not worth the additional pain each user will have with installing the library.

And I don't think I'm using "delayed threads" in any form. Does the term "task" sound like mulit-threading?

USB PD is a protocol that has state machines at its core and many timeouts that start when certain events occurs and require an action when they expire. A USB PD source in particular would have several timeouts running in parallel. A state machine transitions when either an external event occurs, when a timeout expires or sometimes as the result of the application.

Timeouts are implemented using what I call "scheduled tasks". They are managed by a task manager so they can share a single hardware timer. I don't see how multi-threading would help here. The linear execution model of threads isn't a good fit. But it would probably be worthwhile to more explicitly model the state machines.

All tasks are executed in an interrupt context. This is the only way to meet the timing requirements of USB PD in the Arduino framework. The library would look much different if it was built for FreeRTOS or a similar framework.

One of the more confusing aspects is probably how the lower layers (where all the events arrive) communicate with the upper layers. The PHY classes use hard-coded calls like PowerController.onXXX(). The controller calls an event handler it has been passed (in interrupt context) and writes a log to a ring buffer. The sink calls an event handler it has been passed when the caller calls poll(). I guess it could be designed differently, provided it still decouples the time-sensitive code in interrupt context from the non-time sensitive code triggered by the application.

I'm open for changes, in particular for pull requests. I would love new features that benefit users, or refactorings that improve the code without making the library harder to use.

[puzrin]

• On STM32 chips, peripherals and their use of pins can often be mutually exclusive. And the Arduino core also grabs several ones. So it would involve quite an effort to figure out valid combinations and test them.

Sure, that's not trivial. But I think, possible. AFAIK there is a semi-official PHY layer spec by Microchip (it's not on the official USB PD website, but referred). I need some time to think about possible APIs and verify how existing implementations can fit into such APIs.

• Even if peripherals are pluggable, they still require a lot of ifdefs as they won't compile on all platforms.

I see no principal problems with #ifdef, if those are scoped inside concrete phy drivers. My point is - we need more strict code isolation between different abstraction layers. For example, right now timers have ifdef inside. At first glance, moving low-level timer features to PHY or constructor params should decouple code and remove platform dependencies of the policy manager layer. And so on...

I'm less convinced about using ETL. Arduino has no library dependency management. So pulling in a library just for a simple ring buffer is probably not worth the additional pain each user will have with installing the library.

Got it. My point is, when doing architecture refactoring, it's a good idea to operate with well-known programming paradigms. For example - message queues, routers, and others. Anyway, ETL is a good source to refresh available "build blocks".

A principal question is to design a good data flow between components. The concrete implementation is not a principal question - ETL dependency, local ETL copy, or custom implementation (like now). That can be decided later when data flows and APIs are stabilized.

And I don't think I'm using "delayed threads" in any form. Does the term "task" sound like multi-threading?

You are right. I used the wrong term. The point was, that timeouts usually should not be part of such tasks. In terms of architecture, the more explicit place is additional FSM states or "threads". But that's a very generic statement and maybe not suitable for this package. The concrete proposal is to re-verify FMS states/transitions for sure.

Timeouts are implemented using what I call "scheduled tasks". They are managed by a task manager so they can share a single hardware timer. I don't see how multi-threading would help here. The linear execution model of threads isn't a good fit. But it would probably be worthwhile to more explicitly model the state machines.

All tasks are executed in an interrupt context. This is the only way to meet the timing requirements of USB PD in the Arduino framework. The library would look much different if it was built for FreeRTOS or a similar framework.

First, since I did not investigate all current FSM transitions deeply, I can not insist on any concrete approach to code writing. But if you are interested in what I mean - take a look at https://github.com/LarryRuane/protothread

The general concept of "threads" is not about RTOS, it's about 2 things:

1. Writing linear code with async calls
2. Instruments to "wait for events/messages/timeouts"

With protothreads, this can be done with almost zero spending. The difference is - all active semaphores (wait conditions) will be checked on every interrupt, instead of single per interrupt. But that's very cheap. "Threads scheduler" still can be rolled inside interrupts, without extra latencies.

Even if those can not be applied to the whole project, they can be used locally. For example, PHY transactions for FUSB302 consist of multiple async i2c read/write calls linearly.

One of the more confusing aspects is probably how the lower layers (where all the events arrive) communicate with the upper layers. The PHY classes use hard-coded calls like PowerController.onXXX(). The controller calls an event handler it has been passed (in interrupt context) and writes a log to a ring buffer. The sink calls an event handler it has been passed when the caller calls poll(). I guess it could be designed differently, provided it still decouples the time-sensitive code in interrupt context from the non-time sensitive code triggered by the application.

I understand. Let's go from top to bottom. Now, if we are at "architecture inspection" stage, it's convenient to think in terms of "data flow" and "state transitions". When those are more clear/optimal - it will be easy to write the final APIs and decide if callbacks are ok or if more complicated things like "message queues" are needed. I can't say anything certain about the code right now. The only thing I can say from my experience - usually 3 weekly iterations are enough for reworks of such kind (to get final apis for all internals/externals). I'm familiar with the most popular oss pd sink implementations (your one, ralim's, google's), but still need some time to understand all possible FSM transitions for Sinc mode. The spec is a bit big to catch all nuances in a couple of days :)

[puzrin]

https://gist.github.com/puzrin/c7303a45a0341107da21281e0ad2947c

Could you take a look at the plan draft? It is still in progress, but the first 2 steps look doable.

Probably, due notable info exchange traffic, it can be more effective to communicate in Telegram/WhatsApp until things become more stable. If you like this idea - send me a contact to vitaly@rcopen.com.