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

Add ATmega8 and fix some minor bugs with program-avr-spi applet #736

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

turmoni
Copy link

@turmoni turmoni commented Jan 23, 2025

There are two changes here:

  1. Adding the ATmega8 to the database of AVR microcontrollers. erase_time was chosen to match the other entry with an erase_time, since not having one seemed to lead to issues with verifying what had been written, and this one seems to work fine
  2. Fixing up the program-avr-spi applet so it doesn't give a traceback whenever you're doing something with an unidentified device

For an explanation of the second one, see the following output I got before adding the ATmega8:

% glasgow run program-avr-spi -V 5 --pin-reset 0 --pin-sck 1 --pin-cipo 2 --pin-copi 3 identify
I: g.device.hardware: device already has bitstream ID 57b5cdebb6d0d20034246a56c317b332
I: g.cli: running handler for applet 'program-avr-spi'
I: g.applet.program.avr.spi: port(s) A, B voltage set to 5.0 V
I: g.applet.program.avr.spi: device signature: 1e 93 07 (unknown)
Traceback (most recent call last):
  File "/home/dave/.local/bin/glasgow", line 8, in <module>
    sys.exit(run_main())
             ~~~~~~~~^^
  File "/home/dave/git/glasgow/software/glasgow/cli.py", line 942, in run_main
    exit(asyncio.new_event_loop().run_until_complete(main()))
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^
  File "/usr/lib/python3.13/asyncio/base_events.py", line 720, in run_until_complete
    return future.result()
           ~~~~~~~~~~~~~^^
  File "/home/dave/git/glasgow/software/glasgow/cli.py", line 710, in main
    return applet_task.result()
           ~~~~~~~~~~~~~~~~~~^^
  File "/home/dave/git/glasgow/software/glasgow/cli.py", line 659, in run_applet
    return await applet.interact(device, args, iface)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/dave/git/glasgow/software/glasgow/applet/program/avr/__init__.py", line 259, in interact
    if device.erase_time is not None:
       ^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'erase_time'

This is actually the result of two different things that needed to be fixed:

  1. The code was attempting to use the device object before it checked to see if the device existed (so this change brings that further up)
  2. The code that checks for the existence of the device explicitly excludes checking when either called with no action or with the "identify" action, which means that you would still get this error even with 1 being fixed

I don't know if a simple return is what's wanted here, but I assume the point of the check was to not error out on an unidentified device because it's already done everything that's needed for the identify command

@turmoni turmoni requested a review from whitequark as a code owner January 23, 2025 22:01
if device.erase_time is not None:
avr_iface.erase_time = device.erase_time
if args.operation in (None, "identify"):
return
Copy link
Member

Choose a reason for hiding this comment

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

This skips over iface.programming_disable().

@@ -256,12 +256,15 @@ async def interact(self, device, args, avr_iface):
"{:02x} {:02x} {:02x}".format(*signature),
"unknown" if device is None else device.name)
Copy link
Member

@whitequark whitequark Jan 24, 2025

Choose a reason for hiding this comment

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

I'd say add programming_disable after the identify call here, and enable it once we operate on a known device.

Copy link
Author

Choose a reason for hiding this comment

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

Hrm, I'm wondering if it might be worth explicitly calling programming_disable before the return, and making ProgramAVRError also call programming_disable, unless I'm misinterpreting the reason for this suggestion or how the code works (both are entirely possible). I think any exception will leave it in this same state.

Copy link
Member

Choose a reason for hiding this comment

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

unless I'm misinterpreting the reason for this suggestion or how the code works (both are entirely possible)

There's a programming_disable call at the very end that your return was skipping over.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, but I think that any raised ProgramAVRError will also miss the programming_disable call, unless I'm misunderstanding. If it is doing that, would it be worth calling programming_disable on all of the errors and explicitly doing it early for the case where the operation is blank or identify?

Copy link
Member

Choose a reason for hiding this comment

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

How about putting the body of the function into a try..finally block which will call programming_disable()? You'd want the initialization of the interface and the initial programming_enable() command to be outside of the block, of course.

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.

2 participants