6 state list

[EliasMartinezJ]

Added a variable in the YAML file, an array of all the names of the states. Using that variable i replaced sections in the code where it seemed like a good idea to use that variable, rather than using hardcoded numbers.

resolves issue 6

[EliasMartinezJ]

Hi Ian,

Looked into the issue you brought up and made those changes you asked for. Pushed it onto GitHub , if you get a chance could you look over it to make sure everything is ok.

Thanks in advance, Elias Martinez

On Mon, Jan 24, 2022, 12:13 PM Conservation Technology Lab at the San Diego Zoo Wildlife Alliance @.***> wrote:

@.**** requested changes on this pull request.

Elias!

This PR is quite close and, once done, is definitely going to be a good refactoring that gets rid of a bunch of magic numbers and assumptions by various functions and classes.

See my inline notes on some things. My highest level notes are:

1.

I don't think we want the state list to be in the YAML file. The YAML file is meant to contain things a user might want to modify and I don't think the user should be modifying this list. I think this STATE_LIST should just be a constant defined somewhere, probably in dencam.py. Of course, that does mean we need a way to get it to BaseController. Probably the best way is as an argument to BaseController's constructor ala what we are doing with ButtonHandler. Another idea--maybe not a good one--is to add it to the configs dictionary after that dictionary is populated from the YAML file. Which do you think is better? 2.

I ran flake8 on the changed files and it is finding bits and pieces here and there. Are you running your code through a linter?

---

In dencam.py https://github.com/icr-ctl/dencam/pull/35#discussion_r791075716:

@@ -45,16 +45,18 @@ def main():

 flags = {'stop_buttons_flag': False}

-

• State_List = configs['STATE_LIST']

We use all caps for constants which I believe is consistent with PEP8 so this would be STATE_LIST

In dencam/gui.py https://github.com/icr-ctl/dencam/pull/35#discussion_r791086561:

@@ -187,7 +181,7 @@ def prep_fonts(controller): size=-int(scrn_height/9)) fonts['smaller'] = tkFont.Font(family='Courier New', size=-int(scrn_height/12))

• fonts['smallerer'] = tkFont.Font(family='Courier New',
• fonts['smallest'] = tkFont.Font(family='Courier New',

I know "smallerer" might be a little silly :-) but since this is not related to the issue being worked on, it might be best to not change it in this PR. If we did feel it needed addressing, it could make sense to change it in a more "just style"-focused refactor issue/PR.

In dencam/gui.py https://github.com/icr-ctl/dencam/pull/35#discussion_r791087844:

@@ -20,7 +20,7 @@ def init(self, configs, recorder, state): super().init() self.recorder = recorder self.state = state

• self.State_List = configs['STATE_LIST']

Here self.State_List should either be thought off as a class constant and should thus be self.STATE_LIST or as a class variable and should be self.state_list. Which do you think is more appropriate?

In dencam.py https://github.com/icr-ctl/dencam/pull/35#discussion_r791089405:

 def cleanup(flags):

flags['stop_buttons_flag'] = True time.sleep(.1)

 try:
     recorder = Recorder(configs)

• state = State(NUM_STATES)
• number_of_states = len(State_List) + 1

It would be nice to get rid of all the plus 1's and minus 1's. I assume you'd need to modify the State class definition, too, then?

In dencam/buttons.py https://github.com/icr-ctl/dencam/pull/35#discussion_r791090264:

@@ -30,13 +30,13 @@ class ButtonHandler(Thread):

 """

• def init(self, recorder, state, stop_flag):
• def init(self, recorder, state, State_List, stop_flag):

this guy should definitely be all lower case because it is an argument to the constructor

In dencam/gui.py https://github.com/icr-ctl/dencam/pull/35#discussion_r791111171:

@@ -103,14 +103,8 @@ def _update(self):

     self.recorder.update_timestamp()

• if self.state.value == 1:
• self.show_frame('NetworkPage')
• elif self.state.value == 2:
• self.show_frame('RecordingPage')
• elif self.state.value == 3:
• self.show_frame('SolarPage')
• elif self.state.value == 4:
• self.show_frame('BlankPage')
• if self.state.value >- 1 and self.state.value <= 4:

Did you mean to have

• 1

The following seems like more common syntax:

= 0

or >= 1 if that is what you actually wanted

— Reply to this email directly, view it on GitHub https://github.com/icr-ctl/dencam/pull/35#pullrequestreview-861412616, or unsubscribe https://github.com/notifications/unsubscribe-auth/AWUW67R5MXJPAZNLKRZO6UTUXWXGDANCNFSM5MSSF7SQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were assigned.Message ID: @.***>

[EliasMartinezJ]

Oh okay yeah that makes perfect sense actually, I'll implement that 5th "offpage" and push that . And I'll get rid of that extra line in the yaml. We do use the networking page as a starting point, So the traversal would be Networking -> recording -> solar -> preview -> off Let me know if that sounds good to you

On Mon, Jan 24, 2022, 3:37 PM Conservation Technology Lab at the San Diego Zoo Wildlife Alliance @.***> wrote:

@.**** requested changes on this pull request.

See notes. The key thing is that there is a fifth state: the blank and off state. And that the device is meant to start in that state.

In dencam/example_config.yaml https://github.com/icr-ctl/dencam/pull/35#discussion_r791227393:

@@ -17,3 +17,4 @@ FRAME_RATE: 25

0, 90, 180, 270. have not checked whether 90 and 270 crop image or what

CAMERA_ROTATION: 180 +

It would be nice if this file could not change at all i.e. it would be good to get rid of the addition of this extra line

In dencam/buttons.py https://github.com/icr-ctl/dencam/pull/35#discussion_r791230087:

@@ -70,7 +70,7 @@ def run(self): GPIO.cleanup()

 def _set_screen_brightness(self):

• if self.state.value > 0:
• if self.state.value >= 0:

This isn't what we want. We want the zero state to turn off the screen backlight. This came up in the README too: the device is meant to start with a black screen and with the backlight off. There are actually 5 states: that blank state with backlight off, Network page, Recording Page, Solar Page, and Video Preview page (which as you noted in the README is overlayed on a Blank page).

In dencam/buttons.py https://github.com/icr-ctl/dencam/pull/35#discussion_r791232820:

                 self.recorder.start_preview()

• elif self.state.value == 0:
• elif self.state.value == self.STATE_LIST.index("NetworkPage"):

This is also probably not quite right. It is related to my comment on line

73. Maybe the fix is to add a state at the beginning of STATE_LIST called something like "OffPage" ? I haven't thought that through so try thinking it through and if it makes sense maybe do that and make the necessary change here and wherever else.

In dencam/gui.py https://github.com/icr-ctl/dencam/pull/35#discussion_r791236801:

@@ -103,14 +103,8 @@ def _update(self):

     self.recorder.update_timestamp()

• if self.state.value == 1:
• self.show_frame('NetworkPage')
• elif self.state.value == 2:
• self.show_frame('RecordingPage')
• elif self.state.value == 3:
• self.show_frame('SolarPage')
• elif self.state.value == 4:
• self.show_frame('BlankPage')
• if self.state.value >= 0 and self.state.value <= 4:

I think given the issue I am talking about with my other comments here (and if you do add a frontside 5th state to STATE_LIST as I suggested somewhere) that this needs to be

= 1

— Reply to this email directly, view it on GitHub https://github.com/icr-ctl/dencam/pull/35#pullrequestreview-861619839, or unsubscribe https://github.com/notifications/unsubscribe-auth/AWUW67QRQQC5WLHMVARN5EDUXXPDZANCNFSM5MSSF7SQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were assigned.Message ID: @.***>

[icr-ctl]

Cool. The reason we want to start with the off page instead of the networking page, though, is so when the system powers up during a nightly reboot or because the power came back after being down because the batteries ran down etc., the system starts in the low-power state with the screen off.

[EliasMartinezJ]

Got it, that's a good point actually. Pushed the new changes, added an OffPage to the list and now starts on that page for power saving. Also removed another section of hard coding that you brought to my attention. Let me know what you think.

On Mon, Jan 24, 2022, 6:07 PM Conservation Technology Lab at the San Diego Zoo Wildlife Alliance @.***> wrote:

Cool. The reason we want to start with the off page instead of the networking page, though, is so when the system powers up during a nightly reboot or because the power came back after being down because the batteries ran down etc., the system starts in the low-power state with the screen off.

— Reply to this email directly, view it on GitHub https://github.com/icr-ctl/dencam/pull/35#issuecomment-1020736527, or unsubscribe https://github.com/notifications/unsubscribe-auth/AWUW67RZJO2Y4GBYEMMQTN3UXYAWLANCNFSM5MSSF7SQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were assigned.Message ID: @.***>

[EliasMartinezJ]

Hi Ian, Was able to fix that linting issue by creating two index variables, I was getting a line too long error prior, and am now using those to shorten the code length for those lines for the if statement

In dencam/gui.py https://github.com/icr-ctl/dencam/pull/35#discussion_r791965712:

@@ -66,7 +66,7 @@ def _setup(self):

         frame.grid(row=0, column=0, sticky='nsew')

• self.show_frame('RecordingPage')
• self.show_frame('NetworkPage')

For this one I just noticed initial traversal went

Offpage , recording(for a brief moment), networking , solar, then camera preview. When I add this line of code it , traverses normally

Message ID: @.***>

[EliasMartinezJ]

I'm confused on the NUM_STATE global variable, I'm not seeing it on my end. I got rid of that a while ago.

Also the index variable are used to get passed a linting issue, where it throws me "line to long" for that if statement

On Tue, Jan 25, 2022, 5:22 PM Conservation Technology Lab at the San Diego Zoo Wildlife Alliance @.***> wrote:

@.**** commented on this pull request.

In dencam.py https://github.com/icr-ctl/dencam/pull/35#discussion_r792249506:

def cleanup(flags): flags['stop_buttons_flag'] = True time.sleep(.1)

 try:
     recorder = Recorder(configs)

• state = State(NUM_STATES)

This line is gone but the use of the State class remains so I'll chime in. Firstly, the State class wasn't introduced by this PR (it was introduced by me :-) ) so its use/removal is a broader discussion. But I do think Kyra makes a good point that maybe it is overkill.

It was partially intended as placeholder for a Finite State Machine that is more complicated when/if the UI doesn't just simply cycle through a fixed state order. Not sure we'll ever need that at this point, though. It does encapsulate things a little bit still, however, and does automatically loop around so it also encapsulates that mechanic, although potentially that could be accomplished just as well with an itertools.cycle. What I suggest is that if we think we should pursue this, that it get written up as a separate issue where we can follow through on a discussion about it but that for the purposed of this PR we let it remain.

— Reply to this email directly, view it on GitHub https://github.com/icr-ctl/dencam/pull/35#discussion_r792249506, or unsubscribe https://github.com/notifications/unsubscribe-auth/AWUW67VDUJP6VELLKDYA663UX5EGNANCNFSM5MSSF7SQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were assigned.Message ID: @.***>