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 #1279, rtems allow variable length queue msgs. #1280

Closed
wants to merge 2 commits into from

Conversation

thesamprice
Copy link

Checklist (Please check before submitting)

Describe the contribution
A clear and concise description of what the contribution is.
Drops check on size != rtems_size to allow variable length queue messages.
Matches the posix queue implementation.

Testing performed
Steps taken to test the contribution:

  1. Build steps '...'
    Built and ran a non cfs system using posix.
    built and ran the same cfs system using rtems, this fix allowed system to run.

  2. Execution steps '...'
    Ran on RTEMS on microblaze.

Expected behavior changes
A clear and concise description of how this contribution will change behavior and level of impact.

  • API Change: RTEMS queues changed to match posix queues for variable length messages

System(s) tested on
RTEMS / Microblaze

Additional context
Add any other context about the contribution here.

Third party code
N/A

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

@thesamprice thesamprice changed the title fix #1279, rtems allow variable length queue msgs. Fix #1279, rtems allow variable length queue msgs. Aug 20, 2022
@thesamprice
Copy link
Author

Im not sure how to fix the commit message.
On my branch its correct. See link
Somehow the pull request is changing the commit message.

@skliper
Copy link
Contributor

skliper commented Aug 22, 2022

@thesamprice - it was the pull request title (lowercase f on fix) that was causing the job to fail. We haven't figured out yet how to get the job to pick up the corrected title to be able to rerun and have it pass.

Copy link
Contributor

@skliper skliper left a comment

Choose a reason for hiding this comment

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

Maybe make the size verification optional? Just skip the check if passed in size is zero.

@thesamprice
Copy link
Author

thesamprice commented Aug 22, 2022

@skliper
Should this check be moved up into the core code rather than in just rtems?

@skliper
Copy link
Contributor

skliper commented Aug 22, 2022

@thesamprice Copy, I should have looked at the shared code:

if (size < queue->max_size)
{
/*
** The buffer that the user is passing in is potentially too small
*/
*size_copied = 0;
return_code = OS_QUEUE_INVALID_SIZE;
}

Looks like both POSIX and VxWorks implementations allow variable size, so I'm curious why the extra check is in RTEMS. Just concerned someone may be depending on this check... although if it's only in a subset of the implementations that's a pretty fragile dependency.
VxWorks:

status = msgQReceive(impl->vxid, data, size, ticks);
if (status == ERROR)
{
*size_copied = 0;
if (errno == S_objLib_OBJ_TIMEOUT)
{
return_code = OS_QUEUE_TIMEOUT;
}
else if (errno == S_objLib_OBJ_UNAVAILABLE)
{
return_code = OS_QUEUE_EMPTY;
}
else
{
OS_DEBUG("msgQReceive() - vxWorks errno %d\n", errno);
return_code = OS_ERROR;
}
}
else
{
*size_copied = OSAL_SIZE_C(status);
return_code = OS_SUCCESS;
}

Linux:

/* Figure out the return code */
if (sizeCopied == -1)
{
*size_copied = OSAL_SIZE_C(0);
/* Map the system errno to the most appropriate OSAL return code */
if (errno == EMSGSIZE)
{
return_code = OS_QUEUE_INVALID_SIZE;
}
else if (timeout == OS_PEND || errno != ETIMEDOUT)
{
/* OS_PEND was supposed to pend forever until a message arrived
* so something else is wrong. Otherwise, at this point the only
* "acceptable" errno is TIMEDOUT for the other cases.
*/
return_code = OS_ERROR;
}
else if (timeout == OS_CHECK)
{
return_code = OS_QUEUE_EMPTY;
}
else
{
return_code = OS_QUEUE_TIMEOUT;
}
}
else
{
*size_copied = OSAL_SIZE_C(sizeCopied);
return_code = OS_SUCCESS;
}

@skliper
Copy link
Contributor

skliper commented Aug 22, 2022

Maybe instead of just deleting the check (and checking for success twice), it could be updated to match the pattern in POSX/VxWorks. Unless someone comes back with a reason why the check could be required for RTEMS I don't have any issues with making it consistent.

@skliper skliper dismissed their stale review August 22, 2022 14:57

Recalling change request based on further conversation

@thesamprice
Copy link
Author

If someone is relying on this check in rtems, their code will break when going to vxworks / posix.

I would recommend doing the following only if the check is needed.

  • moving the check up into the shared area.
  • and adding some sort of settings flag when its initialized
    OS_FIXED_SIZE_QUEUE
  • only doing the check if OS_FIXED_SIZE_QUEUE is set.

This would keep the existing functionality common across all platforms.

The functionality should be documented also.

I use the queues to route messages to udp/tcp so this rtems check is breaking for me only on rtems.

Ill go ahead and fork osal, and make the change so our system can work on rtems.

@dzbaker dzbaker added CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Aug 25, 2022
@dzbaker
Copy link
Collaborator

dzbaker commented Aug 25, 2022

CCB 25 Aug 2022: @dzbaker will pick up from @skliper

@thesamprice
Copy link
Author

Closing pull request, this will be replaced by the posix mimic version.

@dmknutsen dmknutsen added this to the Draco milestone Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants