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

Fix #219, #93, Add EVS port timestamp and simplify port selection #2295

Merged
merged 2 commits into from
Apr 28, 2023

Conversation

skliper
Copy link
Contributor

@skliper skliper commented Apr 17, 2023

Checklist (Please check before submitting)

Describe the contribution

Testing performed
CI and on local build with time stamp enabled

Expected behavior changes
Adds time stamp when configured to do so

System(s) tested on
CI, Ubuntu 20.04

Additional context
Related but doesn't address (although should make the change simpler):

Third party code
None

Contributor Info - All information REQUIRED for consideration of pull request
Jacob Hageman - NASA/GSFC

@skliper skliper added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Apr 17, 2023
@skliper
Copy link
Contributor Author

skliper commented Apr 17, 2023

Only failure is the overly restrictive commit message format check. Should there be a standard for commits that fix two issues?

@dzbaker dzbaker requested a review from jphickey April 20, 2023 18:03
@CDKnightNASA
Copy link
Contributor

A couple comments:

  1. YYYY/MM/DD HH:MM:SS.MMM adds a lot of bytes to the log, it may make sense to just use HH:MM:SS.MMM or even have a "epoch" at the top of the log then all reports are SS.MMM from T0.
  2. The "ts" command for Unix adds timestamps to streams (surely not microsecond-accurate.)
  3. Instead of compile-time optional, would it be helpful to have it be command-able?

@skliper
Copy link
Contributor Author

skliper commented Apr 20, 2023

  1. YYYY/MM/DD HH:MM:SS.MMM adds a lot of bytes to the log, it may make sense to just use HH:MM:SS.MMM or even have a "epoch" at the top of the log then all reports are SS.MMM from T0.

I only need the format added. If someone requires additional formats they could address in a future issue.

  1. The "ts" command for Unix adds timestamps to streams (surely not microsecond-accurate.)

I don't think this would help my specific need, wouldn't correlate to syslog timing or packet timestamps.

  1. Instead of compile-time optional, would it be helpful to have it be command-able?

I only need it via config. If someone requires commandability they could address in a future issue.

Good suggestions, but given my current requirements this change is all I need. Hopefully the future work can be independent from this ?

@skliper
Copy link
Contributor Author

skliper commented Apr 20, 2023

I also want to get this week's award for addressing the oldest issue (2017!). So there's that :)

@skliper
Copy link
Contributor Author

skliper commented Apr 25, 2023

@dzbaker @dmknutsen - Is there a CCB action on this one or is it pending review?

@dzbaker
Copy link
Collaborator

dzbaker commented Apr 25, 2023

@dzbaker @dmknutsen - Is there a CCB action on this one or is it pending review?

@jphickey is just giving it a quick review to make sure there are no conflicts with other stuff being developed. It should be merged in at the end of this week.

@dmknutsen dmknutsen self-requested a review April 25, 2023 15:35
Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A general comment, in CFE/CFS we have lots of configuration options - configuration option overload, possibly - to the point that we cannot feasibly test them all.

I'm trying to think why we can't just add the time here, and make it "always on"? Is there a use-case where this would break someone's environment somehow? I don't think anyone would rely on not having a timestamp in event text. If we can avoid having yet-another-option that we can't test/maintain both ways, I'd rather just make this the new standard.

@skliper
Copy link
Contributor Author

skliper commented Apr 27, 2023

... just add the time ...

I'll do it! Much preferred from my perspective.

@skliper
Copy link
Contributor Author

skliper commented Apr 27, 2023

@dmknutsen @jphickey - removed option to exclude timestamp in 43270f5

@dzbaker
Copy link
Collaborator

dzbaker commented Apr 28, 2023

@jphickey @skliper @dmknutsen I can fast track this and include it in this next bundle if that makes sense?

Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it....

@dzbaker dzbaker added CCB:FastTrack CCB:Ready Ready for discussion at the Configuration Control Board (CCB) and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Apr 28, 2023
@jphickey
Copy link
Contributor

That being said, I dunno if this should close #93 ... The intent with that old idea is that EVS should call into the PSP for low level output. This is where you can make the 4 "output ports" go to different physical devices (depending on what the board has) and also attach different info per-port. That was the idea at least. Having 4 "output ports" in EVS without an mapping to the lower-level hardware seems to be an incomplete implementation of this, its been that way for years though.

@skliper
Copy link
Contributor Author

skliper commented Apr 28, 2023

This is the one to move to the PSP (at least how I interpret them):

93 just cleans up how the bits are checked and paves the way for 94:

@jphickey
Copy link
Contributor

This is the one to move to the PSP (at least how I interpret them):

* [EVS "output ports" should be a function of the PSP #94](https://github.com/nasa/cFE/issues/94)

93 just cleans up how the bits are checked and paves the way for 94:

* [Clean up EVS ports implementation #93](https://github.com/nasa/cFE/issues/93)

Thanks, yes this makes sense. I'm OK with this closing #93. Maybe someday will get to implement #94 too, I'd still like to see that done.

dzbaker added a commit to nasa/cFS that referenced this pull request Apr 28, 2023
*Combines:*

cFE v7.0.0-rc4+dev287

**Includes:**

*cFE*
- nasa/cFE#2306
- nasa/cFE#2295
- nasa/cFE#2303
- nasa/cFE#2312

Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com>
Co-authored by: Jacob Hageman  <skliper@users.noreply.github.com>
Co-authored by: Haven Carlson <havencarlson@users.noreply.github.com>
@dzbaker dzbaker merged commit c5e27fb into nasa:main Apr 28, 2023
dzbaker added a commit to nasa/cFS that referenced this pull request Apr 28, 2023
*Combines:*

cFE v7.0.0-rc4+dev287

**Includes:**

*cFE*
- nasa/cFE#2306
- nasa/cFE#2295
- nasa/cFE#2303
- nasa/cFE#2312

Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com>
Co-authored by: Jacob Hageman  <skliper@users.noreply.github.com>
Co-authored by: Haven Carlson <havencarlson@users.noreply.github.com>
@dzbaker dzbaker added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) CCB:FastTrack labels May 4, 2023
@dmknutsen dmknutsen added this to the Equuleus milestone May 26, 2023
@skliper skliper deleted the fix219_addevstimestamp branch September 1, 2023 20:04
@skliper skliper restored the fix219_addevstimestamp branch September 1, 2023 20:05
@skliper skliper deleted the fix219_addevstimestamp branch September 1, 2023 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB Equuleus-rc1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add timestamps to EVS logging to stdout Clean up EVS ports implementation
5 participants