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

["Ni, Ray" <ray.ni@intel.com>]

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