Skip to content

feat: add support for a number of exposure-related options #14

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 15, 2020
Merged

feat: add support for a number of exposure-related options #14

merged 3 commits into from
Jul 15, 2020

Conversation

darrylhodgins
Copy link
Contributor

This is related to #8
gregnr
gregnr suggested changes Jul 14, 2020
View reviewed changes
Copy link
Contributor

@gregnr gregnr left a comment

Choose a reason for hiding this comment

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

Excellent work @darrylhodgins! I'm happy to see the shared-args abstraction to eliminate duplication between raspistill & raspivid. Thank you for also following existing conventions.

There are some minor changes I would like to see (noted in this review). In addition to these, can you please link to the new ExposureMode and AwbMode options at the beginning of the API documentation in the readme?
src/lib/shared-args.ts Outdated
/**
* Analog Gain
*/
...(options.analoggain ? ['--analoggain', options.analoggain.toString()] : []),
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's stay consistent with this lib's camel case convention (even though the underlying arg isn't camel cased)

Suggested change
...(options.analoggain ? ['--analoggain', options.analoggain.toString()] : []),
...(options.analogGain ? ['--analoggain', options.analogGain.toString()] : []),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, that was a copy-paste/multi-line edit thing. I'll fix that.
src/lib/shared-args.ts Outdated
/**
* Digital Gain
*/
...(options.digitalgain ? ['--digitalgain', options.digitalgain.toString()] : [])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
...(options.digitalgain ? ['--digitalgain', options.digitalgain.toString()] : [])
...(options.digitalGain ? ['--digitalgain', options.digitalGain.toString()] : [])
src/lib/still-camera.ts Outdated
Comment on lines 17 to 18
analoggain?: number;
digitalgain?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
analoggain?: number;
digitalgain?: number;
analogGain?: number;
digitalGain?: number;
src/lib/stream-camera.ts Outdated
Comment on lines 38 to 39
analoggain?: number;
digitalgain?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
analoggain?: number;
digitalgain?: number;
analogGain?: number;
digitalGain?: number;
README.md Outdated
Comment on lines 247 to 248
- `analoggain: number` - *Default: 0*
- `digitalgain: number` - *Default: 0*
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `analoggain: number` - *Default: 0*
- `digitalgain: number` - *Default: 0*
- `analogGain: number` - *Default: 0*
- `digitalGain: number` - *Default: 0*
README.md Outdated
Comment on lines 286 to 287
- `analoggain: number` - *Default: 0*
- `digitalgain: number` - *Default: 0*
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `analoggain: number` - *Default: 0*
- `digitalgain: number` - *Default: 0*
- `analogGain: number` - *Default: 0*
- `digitalGain: number` - *Default: 0*
README.md Outdated
Comment on lines 384 to 394
- `Off`
- `Auto`
- `Sun`
- `Cloud`
- `Shade`
- `Tungsten`
- `Fluorescent`
- `Incandescent`
- `Flash`
- `Horizon`
- `GreyWorld`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `Off`
- `Auto`
- `Sun`
- `Cloud`
- `Shade`
- `Tungsten`
- `Fluorescent`
- `Incandescent`
- `Flash`
- `Horizon`
- `GreyWorld`
- `AwbMode.Off`
- `AwbMode.Auto`
- `AwbMode.Sun`
- `AwbMode.Cloud`
- `AwbMode.Shade`
- `AwbMode.Tungsten`
- `AwbMode.Fluorescent`
- `AwbMode.Incandescent`
- `AwbMode.Flash`
- `AwbMode.Horizon`
- `AwbMode.GreyWorld`
gregnr
gregnr suggested changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@gregnr gregnr left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. The below change should be all we need now before merging.
@darrylhodgins @gregnr
docs: correct AwbMode enum values
80cadff 
Co-authored-by: Greg Richardson <greg.nmr@gmail.com>
@gregnr
Copy link
Contributor

gregnr commented Jul 15, 2020

Great work. Thanks again!
@gregnr gregnr merged commit d97f4fb into launchcodedev:master Jul 15, 2020
@gregnr gregnr mentioned this pull request Jul 15, 2020
Closed
@darrylhodgins
Copy link
Contributor Author

You're very welcome… I needed a few of these options for my project, so it helps me out too :)
@gregnr gregnr mentioned this pull request Jul 15, 2020
Closed
@darrylhodgins darrylhodgins deleted the exposure-opts branch July 16, 2020 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants

@darrylhodgins @gregnr