Re: [PATCH] Features/Intel/Readme.md: Document meaning of "Complete" for features

["Oram, Isaac W" <isaac.w.oram@intel.com>]

I agree that I cannot think of a good reason that the interface would be in feature packages and the only use and implementation be in board packages.

With respect to fine grain binary modularity, I don't have strong data or a strong intuition as to why attempts at driver level modularity have not worked well.  My intuitions say that it is something like:  we haven't found the right use cases, binary re-use of stable code isn't valuable enough, and if features are too small it is too complicated to use effectively.  
I think that we have emerging use cases around build time, partial updates, and firmware scaling.  By scaling I mean that firmware continues to grow and to control the impacts of growth, it is often nice to break things into smaller pieces that evolve more independently.  To be clear, in this context I mean breaking the monolithic thing into smaller pieces.  My focus is on useful FV full of related features.  I hope we can reduce visible interdependencies, get build time benefits, and eventually validation and update benefits.  It remains to be proven though.

With respect to packages vs directories, I concur that packaging has some advantages.  I am just skeptical that the cost is justified without realizing more developer value for the change.

With respect to AdvancedFeaturePackage abstracting future change.  My request is obtain wide adoption before impacting existing consumers.

Regards,
Isaac

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Thursday, March 12, 2020 8:21 PM
To: Oram, Isaac W <isaac.w.oram@intel.com>; Ray Ni <niruiyu@users.noreply.github.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Chan, Amy <amy.chan@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
Subject: RE: [PATCH] Features/Intel/Readme.md: Document meaning of "Complete" for features

Isaac,
Thanks for the comments. Reply in below.

> -----Original Message-----
> From: Oram, Isaac W <isaac.w.oram@intel.com>
> Sent: Thursday, March 12, 2020 11:29 PM
> To: Ray Ni <niruiyu@users.noreply.github.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; 
> Chan, Amy <amy.chan@intel.com>; Chaganty, Rangasai V 
> <rangasai.v.chaganty@intel.com>
> Subject: RE: [PATCH] Features/Intel/Readme.md: Document meaning of 
> "Complete" for features
> 
> Ray,
> 
> I don't think that this is a desirable rule.
> 
> I want to create feature packages that bundle frequently used together 
> existing capabilities.  See the NetworkFeaturePkg for an example.  I 
> also want to make feature packages for the USB stack, debug capabilities, and the like that are often aggregations of existing modules.

Thanks for reminding me the NetworkFeaturePkg case. NetworkFeaturePkg is a valid case.
I want to add this rule to avoid creating a feature package that only contains header files, but the implementations are in each Board package. Do you agree this should be avoided?
How about:
"A feature package must not contain only interfaces which are implemented by board source code packages."

> 
> The Minimum Platform Architecture spec targets advanced features that 
> are easy to enable for relatively inexperienced developers.  One way 
> of doing that is to leverage the UEFI PI arch and its binary component support features.  The Minimum Platform Architecture aims to use this to enable a use case leveraging Firmware Volumes that looks like:
> 1:  Build NetworkFeaturePkg (this produces an FV, customized via PCD 
> and/or static libraries as needed)
> 2:  Load FV (from shell, by injecting into an existing image using 
> FMMT, Fiano, etc)
> 3:  Use network features and functionality
> 
> The model where the only way people extend a UEFI firmware image is by 
> rebuilding a complete solution needs to end.  It is a misuse of the 
> architecture in my estimation.  We have not had much success with fine 
> granularity component binary use, i.e. individual PEIM and drivers.  
> Perhaps there is too much expertise needed.  Minimum Platform Architecture and Advanced Features aim to improve this by enabling larger granularity binary components that require less UEFI knowledge to use effectively.

Is your concern that binary modularity may be not always practical today? If that's it, I agree with your concerns.
I do find that /Features/Intel/Debugging/Usb3DebugFeaturePkg only contains library. I think the goal is binary modularity. Before that, source modularity is the bottom-line requirement for each feature package.

> 
> I recognize that there is a competing vision that wants to make many 
> small feature packages that are easy to build in or out based on 
> simple PCD feature flags.  As that may improve developer's experience, 
> it is not something I am strongly contesting.  However, I just don't see it as any different than MdeModulePkg.  It is the same strategy, just using packages to organize instead of directories.

The key difference I can see between package and module is that package groups the module and the accordingly public interfaces together. While if putting lots of modules inside a combo package, all the public interfaces (like header files) are together and it's hard to tell which interfaces are used by which modules.

> 
> The other consideration should include that we have a lot of existing 
> users.  I don't want to move existing code around to make usable 
> features.  If we move existing code to create the feature in the first place, we affect all the existing users, often for no immediate benefit.  If features become successful and widely used, then is a good time to refactor the code.
> The difference is that at that time, the change is essentially behind 
> an abstraction and so the change doesn't cause as much pointless work.

AdvancedFeaturePkg is the abstraction layer that aims to hide the future changes.

> 
> Regards,
> Isaac
> 
> 
> -----Original Message-----
> From: Ray Ni <niruiyu@users.noreply.github.com>
> Sent: Thursday, March 12, 2020 5:41 AM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; 
> Chan, Amy <amy.chan@intel.com>; Chaganty, Rangasai V 
> <rangasai.v.chaganty@intel.com>; Oram, Isaac W 
> <isaac.w.oram@intel.com>
> Subject: [PATCH] Features/Intel/Readme.md: Document meaning of 
> "Complete" for features
> 
> Today's document doesn't forbidden creation of a feature package with 
> only interfaces and no code to implement the interfaces. Such feature package is useless.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Amy Chan <amy.chan@intel.com>
> Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
> Cc: Isaac W Oram <isaac.w.oram@intel.com>
> ---
>  Features/Intel/Readme.md | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Features/Intel/Readme.md b/Features/Intel/Readme.md index 
> 9729f90a41..f0923e3d56 100644
> --- a/Features/Intel/Readme.md
> +++ b/Features/Intel/Readme.md
> @@ -18,7 +18,7 @@ document as needed.
>  Advanced features should be:
>  * _Cohesive_, the feature should not contain any functionality unrelated to the feature.
>  * _Complete_, the feature must have a complete design that minimizes 
> dependencies. A feature package cannot directly
> -  depend on another feature package.
> +  depend on another feature package. A feature package must contain module(s) to implement the feature interfaces.
>  * _Easy to Integrate_, the feature should expose well-defined software interfaces to use and configure the feature.
>    * It should also present a set of simple and well-documented standard EDK II configuration options such as PCDs to
>    configure the feature.
> --
> 2.21.0.windows.1