getattr() doesn't work on dicts, use .get() instead

[aarontusko]

getattr() doesn't work on dicts use .get() instead

[FoamyGuy]

@aarontusko did you end up having a different work around or something that made these changes not needed?

[aarontusko]

No. This was my first pull request so I am still learning github, and confidence is not very high. It seemed like it was just being ignored, and it had some "Checks" fail that I don't understand so I just deleted the request.

[FoamyGuy]

@aarontusko sorry we didn't get to it a bit quicker. We appreciate your contribution.

I'm not super familiar with this library and it's usage, so I'm not sure if or to what extent I can test (i'll see if I can learn enough to do some useful testing), but I am definitely willing to help get the PR passing the CI checks if you're interested in re-opening.

Feel free to re-open it if you'd like I've got it on my list to take a closer look into later on tonight.

[FoamyGuy]

@aarontusko thank you for raising this issue and making the PR to resolve it! I apologize again that no one followed up on this PR sooner.

I confirmed the root of this issue and proposed solution are correct with REPL:

Adafruit CircuitPython 7.1.0-beta.0 on 2021-11-12; Adafruit Feather RP2040 with rp2040
>>> 
>>> dict = {"name": "blinka"}
>>> getattr(dict, "name", "default")
'default'
>>> dict.get("name", "default")
'blinka'
>>> dict.get("notexist", "default")
'default'

It looks like only one of the device dictionaries contains the "clock_speed" key: https://github.com/adafruit/Adafruit_CircuitPython_AVRprog/blob/1bb3fbc4c0bf8cd53daae1fccd40ac6bf38bcb94/adafruit_avrprog.py#L58

The rest of them don't have a clock_speed and will end up falling back on the default value passed to getattr() which is _FAST_CLOCK in this case, and that appears to allow them to work successfully.

For the ATtiny13a it does have a clock_speed and it's different from the value of _FAST_CLOCK. It would also end up falling back to the default value since getattr() cannot be used to lookup dictionary key/values. So I assume it would fail since it would be using the wrong clock speed.

I successfully tested the library with the changes from this PR using: https://github.com/adafruit/Adafruit_CircuitPython_AVRprog/blob/main/examples/avrprog_program_uno328.py and confirmed it is still behaving as expected. Since that chip doesn't have clock_speed in it's dictionary it's not expected for this to make a difference.

I don't think I have access to an ATtiny13a so I cannot test that specific case, which is the only one where this fix would specifically be needed if I am understanding the code in adafruit_avrprog.py correctly. But based on the above information and findings from REPL about getattr() vs. dict.get() this solution does make sense and seem correct to me and likely necessary in order to program an ATtiny13a.

It seems that Github is no longer showing the full output from the actions check so I can't see the specific reason that it failed but I am looking into that locally and on a new branch in my fork of the repo. Judging from the relatively small amount of actual code change I think it should be fairly straightforward to get this passing actions and then we can get it merged after that.

@aarontusko if you still have the ability to restore your branch and are willing to do that please do. I will help get actions passing and get this merged.

[FoamyGuy]

I made a new PR with the same changes from this one to try to get a look at why the actions may not have passed. The new PR is passing as-is.

I think that it's likely the reason this one did not originally pass was unrelated to the changes actually from this PR. There have been some changes to PyLint checks across all libraries that started enforcing the use of with: context processors. My best guess is that may have been the reason this didn't originally pass, but it has since been resolved and was unrelated to this PR.

I think it would be great if we can get this branch restored and merge the fix from here.

@aarontusko it seems to me that you did a great job with everything in this PR so congrats on your first github contribution / pull request.

[aarontusko]

@FoamyGuy Yay! Thank you for the time, help and encouragement.

Your example demonstrates the issue and solution perfectly.

The "clock_speed" key is optional in the Boards dicts, so if it is not there the code will just use a default value.

In my case I had two different targets to program. The mega328p @ 7.3728MHz worked fine, but the 644p @ 3.6864MHz needed a slower SPI clock speed. When I added "clock_speed" to the 644p dict it made no change. Obviously because "clock_speed" was a key, and the code was looking for an attribute.

Unfortunately I already deleted my fork, so I don't think I can restore the branch now. I guess I will have to make another fork?

Thanks again

[FoamyGuy]

@aarontusko If you are up for it please do make another fork and add these changes again. I was reading around a bit and it seems like we may not be able to get it to "hook up" back to this PR from a a new fork. But you can create a new PR once you do have a new fork and we'll link it back to this one with a comment so that folks will be able to see the history of it.

[aarontusko]

@FoamyGuy Ok, I made a new fork and pull request. Thanks again for all the help, I appreciate it very much.

[FoamyGuy]

@aarontusko alright new one is merged and release has been made with it. The updated version will appear in the library bundle starting tomorrow and in the meantime can be obtained here on Github.

You're very welcome, I'm happy to help out. Thank you again for contributing this fix and congrats once more on your first contribution and PR :tada:

If in the future you end up making other PRs to this or any of the other CircuitPython libraries and don't get a response within a few business days please feel free to @ mention me in a comment on the PR and I'll get the right set of eyes on to it.