Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for asynchronous console output in shared layer (NG) #245

Closed
skliper opened this issue Sep 30, 2019 · 13 comments
Closed

Add support for asynchronous console output in shared layer (NG) #245

skliper opened this issue Sep 30, 2019 · 13 comments
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Sep 30, 2019

On some target boards, the console output from OS_printf needs to be buffered and sent to a separate task for output. This was enabled on several VxWorks targets through the UTILITY_TASK directive, but generally was not implemented on POSIX or RTEMS targets, where OS_printf was a simple wrapper around printf.

This console output buffer will become necessary in order to support this paradigm for a VxWorks-NG implementation.

The basic framework for doing this is fairly simple, and may provide immediate benefit for any RTEMS or POSIX boards that might want to enable it, too.

@skliper skliper added this to the osal-5.0.0 milestone Sep 30, 2019
@skliper skliper self-assigned this Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Imported from trac issue 222. Created by jphickey on 2019-06-10T12:08:16, last modified: 2019-07-26T15:13:27

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2019-06-11 19:27:03:

Commit [changeset:14e7c45] on branch trac-222-async-console is ready.

Summary:
This makes a "console" device into another top-level resource category in OSAL. In theory this allows multiple consoles, but only one is ever anticipated. But by doing it this way we can re-use all the existing locking semantics offered by the shared layer without adding much code to support this.

The console device has a buffer associated with it, and all data gets written to the buffer. When new data is available in the buffer, the low-level implementation gets notified via an API call. The implementation can then either copy the data directly to the console device or it can wake up a low-priority helper task to process the data.

VxWorks had supported this as a one-off option via the OS_UTILITY_TASK_ON directive. With this change the equivalent is now also available in posix-ng and rtems-ng. It will be necessary for feature parity when a future vxworks-ng is introduced as well.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-06-24 12:22:39:

#including a c file isn't standard practice/style, there's a high bar for justification to allow it here vs more standard mechanisms

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2019-06-25 09:48:57:

Can you clarify what you mean by "more standard mechanisms"?

I'm happy to change this but this is what many of the other modules do already, and it is how the "portable" blocks are intended to be used - it is not limited to this new console service. If there is some way to change this to make it more compliant with the desired style then please advise.

A bit of background -- the "portable" directory contains code blocks that are not self contained compilation units but rather building blocks of code. This allows the same code to be used (shared) among multiple operating systems where the differences are confined to header files and constant definitions, but the code is otherwise the same.

(This general structure was already reviewed as part of "NG" OSAL -- we still could change it if not acceptable, but I am not sure what to change it to)

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by cdknight on 2019-06-25 17:47:54:

In general I like this code change, with the following caveats:

  1. My gut is that the semantics of OS_printf would be "do what's minimally necessary to get args to an OS-provided device". Need to make it clear that OSAL may do queuing and other operations on OS_printf calls.

  2. Are there significant reasons for having the option to not use the utility task model? Sure, there's some performance hit (memory for the queue and more operations per call) but I'm always a little nervous when I see a "#define to switch code paths in the name of performance".

  3. Seems this is creeping into EVS territory, ever-so-slightly. Might want to push the EVS "ports" model down to OSAL, for example. A holistic approach might be in order. See [https://babelfish.arc.nasa.gov/trac/cfs_cfe/ticket/274]

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2019-06-25 21:22:01:

Thanks for the feedback Chris. I like the idea of using this to implement the EVS "output ports" concept, and I think it fits perfectly -- basically each EVS output port is just a different uint32 console ID. This would simplify EVS while actually making the output port feature potentially useful.

To answer comments 0+1 -- the fact that this is even based on a #define at all is just historical. This is what the VxWorks did for OS_printf(), but POSIX and RTEMS were lacking it, probably because POSIX was always seen as a test/debug OS (so it didn't matter) and maybe RTEMS just hadn't gotten there yet.

The main issue that the VxWorks OSAL seemed to be addressing is that printf() can take a long time to pipe the characters out, to the point that it can really interfere with realtime performance. I'm not fond of adding another #define either, but this one was arguably the worst kind because it was already there but only did something on VxWorks, so it wasn't even implemented consistently. This at least makes it consistent.

We could take the #define out completely, and either enable the utility task/buffer all the time, or never enable it, and instead do some extra buffering in CFE_ES (such as in CFE_ES_WriteToSyslog) and insist that those ES calls are used, not OS_printf, if you want to maintain somewhere near real time. But my feeling is that it would have to be the former (always enabled), because the precedent was already set by having this option in VxWorks.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-06-26 13:00:34:

CCB 6/26/19 - Discussed the string copy (gets truncated to buffer size), buffered output via the utility task at a lower priority will get delayed by higher priority tasks.

Also discussed documentation of this behavior (and impact of delays). Can always raise priority of child task to reduce delay, or just replace code with printf for debugging.

Discussed removing option to turn off, no strong objections brought up. Suggestion is to remove option.

Passed initial code review gate (will review updates as part of test review/merge step).

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2019-06-28 16:08:28:

Update: [changeset:48b36e5]

This incorporates feedback from 2019-06-26 CCB review and discussion.

  • The OS_UTILITY_TASK_ON config directive is removed from osconfig.h, and always assumed to be TRUE.
  • Added extra notes in the comments regarding setting the priority of the task to achieve desired balance between timeliness of printf statements and timeliness of running tasks.

The section of osconfig.h now reads as this:
{{{
/*

  • Definitions regarding the "utility task"
  • The utility task is a special background task started
  • by OSAL to move data from calls to OS_printf() to the
  • actual console output device. In former versions of
  • OSAL (4.2.x and below), this was a VxWorks-only option.
  • In the current OSAL, the task is always enabled, but
  • the performance can still be tuned to achieve the
  • desired behavior.
  • Where realtime performance is a concern, the task
  • may be set to a low priority (high number) such that
  • threads calling OS_printf() are not blocked significantly.
  • However, this may delay the actual output from a call to
  • OS_printf() from reaching the console.
  • During debugging, if it is desired to see output from
  • OS_printf() immediately as it occurs, this task can be set
  • to a high priority (low number). This will effectively
  • preempt the task that called OS_printf() until the output
  • is completed.
  • By default this is set to 245 for low priority/delayed
  • output. This still leaves some room for lower priority
  • tasks, if desired.
  • It should not be necessary to change the stack size from
  • the default.
    */
    #define OS_UTILITYTASK_PRIORITY 245
    #define OS_UTILITYTASK_STACK_SIZE 2048
    }}}

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-07-10 13:23:34:

CCB 07/10/2019 - Approved for merge to dev

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2019-07-10 13:56:07:

Now in the "development" branch

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-07-11 10:04:50:

Note [changeset:5b770da] was required to fix errors in the classic build coverage testing.

As part of [changeset:48b36e5] the OS_UTILITY_TASK_ON define was removed from the posix osconfig.h (src/bsp/pc-linux/config/osconfig.h), but since it was defined for the classic build coverage testing in src/unit-test-coverage/vxworks6/osapi-test/osapi_testcase.h, the test case and build code was out-of-sync.

First failure (without this fix) is seen in osapi_testcase_time_int.c, OS_DroppedMessages is not defined. Fix was to remove OS_UTILITY_TASK_ON from osapi_testcase.h. logMsg also needed a stub map.

This change has been marked for CCB review (merged out of cycle to fix the error).

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-07-18 11:28:34:

CCB 7/17/2019 - Code reviewed and approved this fast-tracked merge (was communicated via email prior to merge).

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-07-26 15:13:27:

Milestone renamed

@skliper skliper closed this as completed Sep 30, 2019
@skliper skliper modified the milestones: osal-5.0.0, 5.0.0 Sep 30, 2019
@skliper skliper removed their assignment Sep 30, 2019
jphickey added a commit to jphickey/osal that referenced this issue Aug 10, 2022
Make CFE core apps consistent in their use of the CFE_SB_MsgId type
and with the CFE SB API.  This employs the CFE SB API whenever any
of the following needs to happen:

- Use of a CFE_SB_MsgId_t value within a printf (event, syslog, etc).
- Initialization of a CFE_SB_MsgId_t from an integer value
- Comparison of two CFE_SB_MsgId_t values
- Checking if a CFE_SB_MsgId_t value is within the valid set

A few new macros are introduced, mainly because the inline functions
that already existed for this purpose cannot be used where evaluation
must be done at compile time (e.g. constants, struct initialization).
These are initially just typecasts, but could become more interesting
in future revisions.
jphickey added a commit to jphickey/osal that referenced this issue Aug 10, 2022
Manually merged to resolve conflicts.

From branch jphickey/fix-263-msgid-api:

Fix nasa#245, Consistent use of MsgId type
Fix nasa#263, Expose CFE_SB_IsValidMsgId() API

From branch dmknutsen/issue_240:

Fixes nasa#240, removes MESSAGE_FORMAT_IS_CCSDS ifdefs from code
jphickey added a commit to jphickey/osal that referenced this issue Aug 10, 2022
This makes CFE_SB_MsgId_t to be a safe wrapper around CFE_SB_MsgId_Atom_t,
such that the values cannot be silently/implicitly interchanged with other
integers.

This enforces that the MsgId/Value conversion helpers must be used when
conversion to/from integers is intended.
jphickey pushed a commit to jphickey/osal that referenced this issue Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant