Add PiCamera component #9
Conversation
kevin-f-ortega
left a comment
kevin-f-ortega
left a comment
There was a problem hiding this comment.
Looks good. I've added my review comments. Also, could you remove the .sdd.md.swp file from this PR?
| @@ -0,0 +1 @@ | |||
| add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/Components/PiCameraManager/") | |||
There was a problem hiding this comment.
Remove /PiCameraManager/ for here. Should be
add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/Components")
| // C++11 compatible means we're using system() calls as opposed to the C++17 libcamera API | ||
| // if the photo capture has succeeded, update all tracking variables | ||
| if (captureImageWithSystemCommand(filename, errorMsg)) { | ||
| U32 filesize = getFileSize(filename); // get the actual file size |
There was a problem hiding this comment.
Instead of defining your own getFileSize function, you should use Os::FileSyste::getFileSize. See here https://github.com/nasa/fprime/blob/devel/Os/FileSystem.hpp#L355
|
|
||
| void PiCameraManager::DOWNLOAD_IMAGE_cmdHandler(FwOpcodeType opCode, U32 cmdSeq) { | ||
| // Check if we have a captured image | ||
| if (m_lastCapturedFile.empty() || !fileExists(m_lastCapturedFile)) { |
There was a problem hiding this comment.
Instead of using your own fileExists function, you should use Os::FileSystem::exists. See here https://github.com/nasa/fprime/blob/devel/Os/FileSystem.hpp#L280
| U32 PiCameraManager::getFileSize(const std::string& filepath) { | ||
| // use stat() data type call to get the file information | ||
| struct stat stat_buf; | ||
| // stat() will return bite size on success, fills stat_buff with file details, will return 0 upon failure | ||
| return (stat(filepath.c_str(), &stat_buf) == 0) ? static_cast<U32>(stat_buf.st_size) : 0; | ||
| } | ||
|
|
||
| bool PiCameraManager::fileExists(const std::string& filepath) { | ||
| // use stat() to check if file exists (we don't need any file info for this) | ||
| struct stat buffer; | ||
| // retun 0 if the file exists and you can accesible it, else non-zero | ||
| return (stat(filepath.c_str(), &buffer) == 0); | ||
| } |
There was a problem hiding this comment.
Should delete these and use the Os::FileSystem ones instead
| //! Get file size in bytes | ||
| U32 getFileSize(const std::string& filepath); | ||
| //! Check if file exists | ||
| bool fileExists(const std::string& filepath); |
| this->log_ACTIVITY_LO_CAMERA_ENABLED_EVENT(); | ||
|
|
||
| // Update telemetry channel (reports specific parameters) | ||
| this->tlmWrite_CAMERA_ENABLED(true); |
There was a problem hiding this comment.
Use m_cameraEnabled instead of true
| this->log_ACTIVITY_LO_CAMERA_ENABLED_EVENT(); | ||
|
|
||
| // Update telemetry channel | ||
| this->tlmWrite_CAMERA_ENABLED(true); |
There was a problem hiding this comment.
Use m_cameraEnabled instead of true
| } | ||
|
|
||
| // generate a unique filename with a timestamp | ||
| std::string filename = generateImageFilename(); |
There was a problem hiding this comment.
I would recommend to avoid using std::string and using F Prime's Fw::FileNameString
| } | ||
|
|
||
| // Takes a photo | ||
| void PiCameraManager::CAPTURE_PHOTO_cmdHandler(FwOpcodeType opCode, U32 cmdSeq) { |
There was a problem hiding this comment.
I would recommend adding an argument for the filename. This filename would be used to store the captured photo instead of generating one in the moment.
| } | ||
| } | ||
|
|
||
| void PiCameraManager::DOWNLOAD_IMAGE_cmdHandler(FwOpcodeType opCode, U32 cmdSeq) { |
There was a problem hiding this comment.
Add an argument for the filename that you want to download.
|
@maddywright3 Any update on this? |
|
@kevin-f-ortega is it okay if I step in to take care of this PR? |
|
Hi @kevin-f-ortega this is all completed with the requested changes now. I have made a new commit. |
4 participants
No description provided.