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

Improvements to openni_pcd_recorder #672

Merged
merged 0 commits into from
May 16, 2014

Conversation

j-oberlaender
Copy link
Contributor

The recorder now has command-line support for device IDs just like openni_viewer.
Also, a problem with free memory and buffer size calculation was fixed.

@jspricke
Copy link
Member

Could you exclude 6b44c54 from this pull request? I think it needs further discussion. Rest is fine, thanks!

@j-oberlaender
Copy link
Contributor Author

Thanks for the feedback! I created a separate pull request for the all but 6b44c54.
For me, 32-Bit systems with a PAE kernel are a regular reality, and since it is easy to support those systems as well I think we should try to report correct memory and buffer sizes for them. Note that currently, the PCD recorder outputs nonsensical numbers like this:

Total available memory size: 579102311809286765MB.

(This of course is because the print statement expects a %llu, but the variable given is not a 64-bit one.) Because the variable can not hold the true memory size, the calculated default buffer size is basically random.

Of course, if my commit breaks something on ARM we need a different solution. Unfortunately I currently don't have an ARM system at my disposal to check that. I did try to keep everything compatible by using std::size_t where it matters. I only force the actual memory size calculation to use 64-bit unsigned integers in case there is more total memory than a size_t can hold.

@jspricke
Copy link
Member

Hm.. looks like we are using uint64_t already in a couple of places, So maybe it's ok (let's wait for the others to comment). Could you clean up this pull request to only contain the type change (use git rebase -i and git push -f)?

getTotalSystemMemory ()
{
size_t memory = std::numeric_limits<size_t>::max ();
uint64_t memory = std::numeric_limits<uint64_t>::max ();
Copy link
Member

Choose a reason for hiding this comment

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

I guess this should be:
uint64_t memory = std::numeric_limits<size_t>::max ();

@j-oberlaender
Copy link
Contributor Author

I guess what you meant to write was std::numeric_limits<size_t>::max () (which got swallowed by the Markdown parser)?

Yeah, you could use the size_t maximum, but that should not make a difference since the value is limited to std::numeric_limits<size_t>::max () in line 82 anyway.

Got back from my lunch break, I'll clean up and squash things now.

@jspricke
Copy link
Member

Yes, that's what I meant and you are right that it doesn't make a difference, only that it makes more sense to me and wouldn't break that easy if we change something later ;).

@j-oberlaender
Copy link
Contributor Author

I'll clean this one, and rebase it, once we're ok with #673.

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.

2 participants