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

Silence logic around error event broken on semaphore timeout #377

Closed
2 tasks done
skliper opened this issue Apr 6, 2023 · 0 comments · Fixed by #379
Closed
2 tasks done

Silence logic around error event broken on semaphore timeout #377

skliper opened this issue Apr 6, 2023 · 0 comments · Fixed by #379
Assignees
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Apr 6, 2023

Checklist (Please check before submitting)

  • I reviewed the Contributing Guide.
  • I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
If the semaphore times out there's no attempt to allocate a buffer. If a buffer isn't allocated and silent is false, an error event is sent claiming there was no buffer available.

if (OS_ObjectIdDefined(c->sem_id))
{
os_status = OS_CountSemTimedWait(c->sem_id, 0);
}
else
{
os_status = OS_SUCCESS;
}
/* Allocate message buffer on success */
if (os_status == OS_SUCCESS)
{
CF_AppData.engine.out.msg = CFE_SB_AllocateMessageBuffer(offsetof(CF_PduTlmMsg_t, ph) + CF_MAX_PDU_SIZE +
CF_PDU_ENCAPSULATION_EXTRA_TRAILING_BYTES);
}
if (!CF_AppData.engine.out.msg)
{
c->cur = t; /* remember where we were for next time */
if (!silent)
{
CFE_EVS_SendEvent(CF_EID_ERR_CFDP_NO_MSG, CFE_EVS_EventType_ERROR,
"CF: no output message buffer available");
}
success = false;
}

This silent logic doesn't make any sense to me, since it's passed in as 0 from all the non-file data PDUs but 1 for data. If it was intended for the allocate buffer why only non-data PDUs? I doubt it was ever intended for the semaphore timeout.

To Reproduce
I saw it when waiting for the semaphore to send an Eof PDU. Could probably see it on the metadata send, but I initialize w/ a nonzero sem count.

Expected behavior
No event on semaphore timeout, this is nominal behavior for flow control. TBH I'm not a huge fan of a possible flooding event on the failure to get a buffer. I'd rather see a combined approach of a counter and probably a single event sent at the maximum rate of each HK cycle only when the counter increments.

Code snips
See above.

System observed on:
Ubuntu 20.04

Additional context
None

Reporter Info
Jacob Hageman - NASA/GSFC

@skliper skliper added the bug label Apr 6, 2023
@skliper skliper self-assigned this Apr 6, 2023
skliper added a commit to skliper/CF that referenced this issue Apr 7, 2023
dzbaker added a commit that referenced this issue Apr 13, 2023
Fix #377, Remove error event on nominal semaphore timeout
@dmknutsen dmknutsen added this to the Equuleus milestone May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants