Add exposure mode (auto/manual) for Pupil Cam 2 #1210
Conversation
There was a problem hiding this comment.
Hey @ChingT
This is great work! I added a few naming proposals with the goal to improve consistency in readability throughout our code base.
I am not 100% happy yet with my proposal. Please let me know what you think about it and if you would like to change my proposal.
| try: controls_dict['Auto Exposure Mode'].value = 1 | ||
| except KeyError: pass | ||
| # try: controls_dict['Auto Exposure Mode'].value = 1 | ||
| # except KeyError: pass |
There was a problem hiding this comment.
Were these lines commented for testing purposes or can they be deleted?
| self.check_freq = 0.1/3 | ||
| self.last_check_timestamp = None | ||
|
|
||
| def cal_exposure_time(self, frame): |
There was a problem hiding this comment.
I would like to refactor the name to calculate_based_on_frame(). This would avoid redundant labeling.
|
|
||
|
|
||
| class GetExposureTime(object): |
There was a problem hiding this comment.
Even though PEP8 would agree with this naming it would be more consistent with all other class names in this project to rename this function to Get_Exposure_Time.
Additionally, I would like to propose to rename this to simply Exposure_Time. Together with the comments below this would yield this naming:
preferred_exposure_time = Exposure_Time(max_ET, frame_rate)
target = preferred_exposure_time.calculate_based_on_frame(frame)| self.checkframestripes = None | ||
| self.get_exposure_time = None |
There was a problem hiding this comment.
I would suggest to rename this instance variable to preferred_exposure_time
|
|
||
| def frame_rate_getter(): |
There was a problem hiding this comment.
Is it intended that the frame rate selector has been removed?
fix errors
|
@papr Thank you for the comments. I think they are nice proposals! |
No description provided.