RaspiStill - DateTimeMilliseconds, USR1 Signal Handling#708
RaspiStill - DateTimeMilliseconds, USR1 Signal Handling#708sebmueller wants to merge 2 commits intoraspberrypi:masterfrom
Conversation
Adding Datetime with Millisecond, removing Delay in Timelapse, adding USR1 Signal handling
|
You need to get rid of the whitespace changes - they are obscuring the functionality changes. |
|
I reformated the file. Please have a look if it is better |
6by9
left a comment
6by9
left a comment
There was a problem hiding this comment.
Comments on the changes inline.
You've got 3 changes intermingled, so reviewing what each is doing is difficult. Please create it as 3 commits rather than 1.
There are also still a number of whitespace changes, and commented out code. Neither add to the readability.
I have no idea what the issue was with USRSIG1 as you've not described it, therefore it's not possible to review sensibly.
| { | ||
| mode_t target_mode = 0777; | ||
| if (mkdir(dname, target_mode) == 0) | ||
| chmod(dname, target_mode); |
There was a problem hiding this comment.
Do we need to chmod after mkdir? My reading is that mkdir will have already set those permissions.
| //create folder, if not exists | ||
| if (stat(dname, &st) == -1) | ||
| { | ||
| mode_t target_mode = 0777; |
There was a problem hiding this comment.
0777 is incredibly open. 0755 would be more normal.
| vcos_sleep(CAMERA_SETTLE_TIME); | ||
| { | ||
| //vcos_sleep(CAMERA_SETTLE_TIME); | ||
| } |
There was a problem hiding this comment.
If it's not needed, then remove it. Commenting out leaves questions as to why it is there.
| */ | ||
|
|
||
| MMAL_STATUS_T create_filenames(char** finalName, char** tempName, char * pattern, int frame) | ||
| MMAL_STATUS_T create_filenames(char **finalName, char **tempName, char *pattern, int frame) |
| timeinfo = localtime(&rawtime); | ||
|
|
||
| *finalName = malloc(100); | ||
| *tempName = malloc(100); |
There was a problem hiding this comment.
Can we not use asprintf here, same as create_filenames, instead of mallocing a random length buffer that we're not validating is big enough.
asprintf(*finalName, "%s/img_%04d%02d%02d_%02d%02d%02d_%04d.jpg", dname, timeinfo->tm_year + 1900, timeinfo->tm_mon + 1, timeinfo->tm_mday, timeinfo->tm_hour, timeinfo->tm_min, timeinfo->tm_sec, ((int)now.tv_nsec / 1000000)); should do it (totally untested!)
| strcat(*finalName, "/"); | ||
| strcat(*finalName, *tempName); | ||
|
|
||
| if (0 > asprintf(tempName, "%s~", *finalName)) |
There was a problem hiding this comment.
I think you've just leaked the 100 bytes you malloc'ed as *tempName.
| if (status != MMAL_SUCCESS) | ||
| if (state.datetime) | ||
| { | ||
| status = create_filenames_datetime(&final_filename, &use_filename, state.common_settings.filename); |
There was a problem hiding this comment.
The help text for -dt / --datetime says
Replace output pattern (%d) with DateTime (MonthDayHourMinSec)
You're no longer doing that, so it's a change in behaviour. Those relying on the defined behaviour are therefore likely to see their installations break. This really needs to be a new option (with documentation).
3 participants
Adding Datetime with Millisecond, removing Delay in Timelapse, adding USR1 Signal handling