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

I 888 mean auto #997

Merged
merged 15 commits into from
Feb 16, 2022
Merged

I 888 mean auto #997

merged 15 commits into from
Feb 16, 2022

Conversation

AndreasLMeg
Copy link
Collaborator

  • Mode mean: now supports AutoGain (on/off), AutoExposure (on/off) and different values for day and night
  • The mean value can also be defined differently for day and night.

@AndreasLMeg AndreasLMeg linked an issue Feb 3, 2022 that may be closed by this pull request
Copy link
Collaborator

@EricClaeys EricClaeys left a comment

Choose a reason for hiding this comment

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

@andreaous. Do you want me to approve this PR?

@AndreasLMeg
Copy link
Collaborator Author

@EricClaeys @linuxkidd - I would be very grateful if you could do a review, test and approve

Copy link
Collaborator

@EricClaeys EricClaeys left a comment

Choose a reason for hiding this comment

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

@AndreasLMeg
In config.sh.repo, can you add "-daymean 0.3" to line 69?

capture_RPiHQ.cpp:
Lines 1067 and 1073 set myModeMeanSetting.mode_mean = true. What if the user has -nightmean set but not -daymean (or vice versa?) Where is myModeMeanSetting.mode_mean set to false? Possibly lines 1067 and 1073 should be deleted and myModeMeanSetting.mode_mean set to true or false where the "current" variables are set, like "currentAutoExposure = asiDayAutoExposure;"?
Minor: on lines 1344 and 1347 in capture_RPiHQ.cpp, "Set" should be "Sets". On line 1347, delete "-day".
Lines 1537-1541. Can these only be displayed if either day or night mean (or both) is set? I've been trying to not add log entries for things that don't apply, for example, on ZWO I don't display the cooling temperature for cameras that don't support cooling, since it doesn't apply.

mode_RPiHQ_mean.h:
Line 5: "-mean-value-day" should be "-daymean" ?
Line 16: "exposureis" should be "exposure is".
Line 28: Should there be a default "dayMean" ?

mode_RPiHQ_mean.cpp:
Line 2: "-mean-value-day" should be "-daymean" ?
Line 278 casts exposure_us and US_IN_SEC to "(double)" but line 270 doesn't. Does that matter?
Line 273 used "(double)" but line 281 doesn't.

@EricClaeys
Copy link
Collaborator

@AndreasLMeg, @linuxkidd,
I haven't had a chance to test yet. I've been on the road for a week and my Pi is currently is a box. I hope to have it working again in a day or two.

@AndreasLMeg
Copy link
Collaborator Author

@EricClaeys Thank you very much for the review !

With the next commit:

  • minor changes: done
  • I know I used too much casts - because I'm not sure everytime what the system does... (in the future I will make unittests)
  • some defaults for mode mean are defined (nightMean, p0, p1, p2, threshold)
    ** If no parameter is used mode mean is deactive and you won't get the info about the parameter
    ** if any of the parameters are used (extra parameter): mode mean is active (other parameters have default values)
    ** if no dayMean is defined, this value is set to nightMean

@AndreasLMeg
Copy link
Collaborator Author

sorry make such a mess - but you can use "Squash" to merge this PR

@EricClaeys
Copy link
Collaborator

@AndreasLMeg Can you walk me through what happens if someone only wants to use the meanmode at night or during the day? Will the code handle that? What variable(s) would they set?
I think we need to allow for that and I would suggest that setting -nightmean turn it on for night only and -daymean turn it on only for daytime.

@EricClaeys
Copy link
Collaborator

@AndreasLMeg If modemean is set, how is that different from auto exposure? I assume we are allowing auto exposure without modemean, and modemean without auto exposure? If true, we'll need to document what the differences are.

@AndreasLMeg
Copy link
Collaborator Author

AndreasLMeg commented Feb 11, 2022

@EricClaeys I hope this will explain

I didn't do or adjust the old behavior.
And I won't put any energy into it either, otherwise there will certainly be issuse "I had allsky running for 5 years without any problems and now nothing works anymore - such a bad program....."

grafik

@EricClaeys
Copy link
Collaborator

@AndreasLMeg
Shouldn't the two blue items be "(0.5, exposure change only)" and "(05, exposure and gain change)" respectively?

I am testing this code now and will let it go overnight and tomorrow during the day.
I think we need to implement max auto-exposure for day and night like the ZWO has - I have my night exposure set to 25 seconds and the algorithm won't increase it past that. It should go up to the max auto exposure values. We can do that after this PR is done.

Capture

@AndreasLMeg
Copy link
Collaborator Author

Shouldn't the two blue items be "(0.5, exposure change only)" and "(05, exposure and gain change)" respectively?

Oh yes, copy paste mistake, sorry.

@EricClaeys
Copy link
Collaborator

Hi @AndreasLMeg In cases where either day or night ModeMean is on but not both, where in the code is it turning it on and off?

@EricClaeys
Copy link
Collaborator

@AndreasLMeg I believe the "==" on line 446 in capture_RPiHQ.cpp should be "!=". If we're doing any ModeMean'ing we want a short --timeout. The way it is now, we only get the short timeout if mean_auto is off.

@AndreasLMeg
Copy link
Collaborator Author

AndreasLMeg commented Feb 12, 2022 via email

@AndreasLMeg
Copy link
Collaborator Author

@AndreasLMeg I believe the "==" on line 446 in capture_RPiHQ.cpp should be "!=". If we're doing any ModeMean'ing we want a short --timeout. The way it is now, we only get the short timeout if mean_auto is off.

I have to check this - it's so far away....

Copy link
Collaborator

@EricClaeys EricClaeys left a comment

Choose a reason for hiding this comment

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

@AndreasLMeg We can add the maxexposure values in a separate PR. We'll also need to update the settings_RPiHQ.json and camera_options_RPiHQ.json in allsky-portal to include the options, then remove the comment about -daymean and -nightmean from config.sh.

@AndreasLMeg AndreasLMeg merged commit 9e8c05b into AllskyTeam:master Feb 16, 2022
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.

capture_RPiHQ.cpp: mean issues
2 participants