Ralim / usb-pd

USB-PD driver stack for the FUSB302
Apache License 2.0
124 stars 15 forks source link

Questions about project status #1

Closed puzrin closed 3 years ago

puzrin commented 3 years ago

Hi again! Could you share code status and your plans?

  1. How stable is current code?
  2. What do you think about rewrite to protothreads? Will you accept API change proposals or PR?
  3. Would be nice to support PlatformIO (library distribution).

Code review notes:

Of cause, i could create fork to implement my vision of "ideal opensource usb-pd library". But i'd prefer to collaborate if possible. What do you think?

Ralim commented 3 years ago

100% happy to collaborate.

This is actually a longer term plan to be an eject out of the IronOS project. This is not actually finished ( I have changes to pull over).

Plan is to pull out threads to just function calls (so you can use your choice of library to create) and for cross thread notifications to be pulled out to a macro for user implementation.

In terms of not handling driver callbacks, I expect that to be handled by the user implementation of the read/write buf functions. You can perform any thread/hardware awareness there was my plan?

Open to ideas on restructure, but have found LTO not the best at removing indirection on storing points to functions inside classes.

1000% happy to collaborate into a library on platformio.

puzrin commented 3 years ago

In terms of not handling driver callbacks, I expect that to be handled by the user implementation of the read/write buf functions.

read/write buf functions should say somehow "i'm finished data transfer". This cam be done in 3 major ways:

  1. Invoke callback
  2. Send message (very close to callback, but different signature)
  3. Hidden by RTOS (function looks sync, but OS switch context inside)

If you say, user-implemented functions have sync signature, the only choice is (3)

Open to ideas on restructure, but have found LTO not the best at removing indirection on storing points to functions inside classes.

I don't know your hardware limits, but in general, right architecture is more important than saving some bytes. If you really have lack of flash/ram - let me know (i will take into account). Everything is discuss-able, and i never had situation when it was impossible to find satisfying approach. For example, cpp can use templates and so on.


At first glance, it would be enough to have .tick() function, called by timer interrupt every 1ms, instead of threads. The rest will organized by protothreads macroses. That will cause up to 1ms extra lag to responses, but IMO that's acceptable price for flexibility.

About collaboration. I'm software architect, it's my strong side to design optimal data flows and API-s. My weak side is to debug hardware :). So, if you have something working at bare metal, i can transform it to something "desired" (and i will avoid need to debug).

I would take 1-2 weeks to study details and propose refactored API draft.

Ralim commented 3 years ago

In terms of not handling driver callbacks, I expect that to be handled by the user implementation of the read/write buf functions.

read/write buf functions should say somehow "i'm finished data transfer". This cam be done in 3 major ways:

1. Invoke callback

2. Send message (very close to callback, but different signature)

3. Hidden by RTOS (function looks sync, but OS switch context inside)

If you say, user-implemented functions have sync signature, the only choice is (3)

I'm not trying to be against this; but I dont see why you would use 1/2 when 3 is cleaner to this code and often trivial when using an rtos based implementation?

Open to ideas on restructure, but have found LTO not the best at removing indirection on storing points to functions inside classes.

I don't know your hardware limits, but in general, right architecture is more important than saving some bytes. If you really have lack of flash/ram - let me know (i will take into account). Everything is discuss-able, and i never had situation when it was impossible to find satisfying approach. For example, cpp can use templates and so on.

True, goal here is minimal indirection to help the flash cache. Open to discussion, but this code is very much designed as a singleton in a system for only a single FUSB302 at the moment

At first glance, it would be enough to have .tick() function, called by timer interrupt every 1ms, instead of threads. The rest will organized by protothreads macroses. That will cause up to 1ms extra lag to responses, but IMO that's acceptable price for flexibility.

1ms is generally ok so long as its only 1ms. Some chargers only give 20ms for the entire transaction to complete. (This should generally be fine). Can definitely split into tick functions for these.

About collaboration. I'm software architect, it's my strong side to design optimal data flows and API-s. My weak side is to debug hardware :). So, if you have something working at bare metal, i can transform it to something "desired" (and i will avoid need to debug).

I would take 1-2 weeks to study details and propose refactored API draft.

If debugging hardware is not your strong point, PD is a mess 🙃 I've seen more not-to-spec than i have to-spec chargers

I would be interested to see what your proposal is, though also probably I would advise waiting if you can for a little bit. I'm currently focusing on the MHP30 bring up, but then after that coming back to PD is the plan. Once thats done this should be in a "stable" place; and when I would be far more open to re-arranging the code a lot more.

Ralim commented 3 years ago

Sorry I meant to add in addition to this; If you do think of a more ideally API for this (just do a dot point draft, nothing fancy) and throw it here, I'll try and shape closer to that as I work at the least :)

Ralim commented 3 years ago

When this merges expect me to be poking this more in earnest.

puzrin commented 3 years ago

I'm not trying to be against this; but I dont see why you would use 1/2 when 3 is cleaner to this code and often trivial when using an rtos based implementation?

Answer is obvious. Library, useable with and without RTOS is more flexible than library, useable with RTOS only :). We can not know user requirements and preferences. It's not good practice to force user use RTOS if he not wish to do so. Also, using protothreads simplifies portage abstraction layer significantly.

Of cause, i don't mean we should pay with messy code for such flexibility. USB-PD code with protothreads will be almost as nice as with RTOS, and easy to maintain. I have some practice with this, and i can do such refactoring to prove.

1ms is generally ok so long as its only 1ms. Some chargers only give 20ms for the entire transaction to complete. (This should generally be fine).

To be more precise, +1 tick per each data transfer call. Some functions to 2-3 calls per message. That's the worst estimate. But probably can be improved. Or ticks frequency can be increased. Anyway, that should not become fatal problem.

If debugging hardware is not your strong point, PD is a mess upside_down_face I've seen more not-to-spec than i have to-spec chargers

That's my reasons for collaboration. I can do things i like, where i have good experience and avoid things i don't like :). Everybody should win.

I would be interested to see what your proposal is, though also probably I would advise waiting if you can for a little bit. I'm currently focusing on the MHP30 bring up, but then after that coming back to PD is the plan.

No problem. Result of API design is pure list of function signatures (+ may be templates skeletons) with verified data flows. The second checkpoint is to verify "rewriting to protothreads is possible" - existing code is enough for that.