Video: Correct use of global variable

[patacongo]

Reported to me via email:

The v4l2-like layer (video.c) uses a global variable inappropriately. A global variable is used instead of creating a struct in /boards/ and passing it. So we have g_video_devops->set_buf(...) for example
g_video_devops being declared in video.h: FAR const struct video_devops_s *g_video_devops;

And being used in the CXD56xx board here: boards/arm/cxd56xx/spresense/src/cxd56_bringup.c: g_video_devops = isx012_initialize();

Wouldn't it be better to create the pointer in boards and pass it as argument ? Using global variables at inter-module interfaces is not good design and anathema to embedded systems. From the INVIOLABLES.txt:

Modular Architecture
--------------------

  o The internal modular architecture of the OS must be maintained.
  o This means formalizing and documenting all internal interfaces (in the
    porting guide), minimal use of global variables at the interface, and
    only well defined functional interfaces.

[jerpelea]

I am looking into it ASAP

[patacongo]

A related issus the the use, or rather the lacke of the use of the video driver initialization function: video_initialize().

That function should be called from the board bringup() functions but it is not. In fact, it is not called from anywhere in the OS. Searching a little, we find that it is insanely called from the Sony SDK applications directly. That is very bad design: Calling directly into the OS from application bypassing the portable, POSIX interface is strictly forbidden. From the INVIOLABLES.txt:

Strict POSIX compliance
-----------------------

  o Strict conformance to the portable standard OS interface as defined at
    OpenGroup.org.
  o The portable interface must never be compromised only for the sake of
    expediency.
  o Expediency or even improved performance are not justifications for
    violation of the strict POSIX interface.

My recommendation is:

1. Remove the direct call to video_initialize() from the SDK application. It is not needed.
2. Add the call to video_initialize() to the spresense bringup() logic.
3. Remove the (now extern'ed) global variable g_video_devops from include/nuttx/video/video.h and mark the g_video_devops global variable in drivers/video/video.c as static
4. Add a : FAR const struct video_devops_s *devops argument to video_initialize()
5. Provide the video_devops_s instance to the driver with video_initialize() in the spresense bringup() function.

The current situation is intolerable and must be corrected!

[jerpelea]

I am sorry for the long response time and thanks for the suggestion.
I will implement and test the suggestion.

[jerpelea]

@patacongo can we close the issue?