Skip to content

First pass at adding type annotations #32

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

First pass at adding type annotations #32

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 22 additions & 14 deletions adafruit_avrprog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,16 @@
__version__ = "0.0.0+auto.0"
__repo__ = "https://github.com/adafruit/Adafruit_CircuitPython_AVRprog.git"

import digitalio
Copy link
Member

Choose a reason for hiding this comment

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

If you're just using something for typing like this library or the microcontroller import below, you can put that in the try/except block guarded by a first import of typing. The logic is as follows:

CircuitPython actually doesn't care about type annotation syntax as a method of saving space, and likewise doesn't store the information gleaned by parsing it. The effect is that it's basically anything goes, even having unbound types (using a variable that hasn't been assigned). Note that this only works in CircuitPython - CPython does care, and will get very mad if it can't resolve a type annotation.

So, in order to save RAM on the microcontroller, we avoid import items that only get used as a part of type annotation since that information isn't used or stored anyway. The way this is achieved is by using the typing import - if we're in a microcontroller environment, it fails and skips importing anything else in the block. But if it's Blinka, it will succeed and continue to import anything else in that block that is required for type annotations.

So! Since this and the following are in that category of inputs, let's put them after the import of typing in that block.

import microcontroller
from digitalio import Direction, DigitalInOut

try:
import typing
Copy link
Member

Choose a reason for hiding this comment

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

pylint warns against unused imports, since it figures you can just remove it. However, this goes against our strategy of using it as a sort of import guard, so in cases where we're actually not importing anything from typing, we can disable the pylint warning with a comment it parses:

    import typing  # pylint: disable=unused-import

However, I have some suggestion below that will make use of things imported from typing, so we won't need to use that strategy in the end.


except ImportError:
pass

_SLOW_CLOCK = 100000
_FAST_CLOCK = 1000000

Expand Down Expand Up @@ -89,7 +97,7 @@ class Boards:
_spi = None
_rst = None

def init(self, spi_bus, rst_pin):
def init(self, spi_bus, rst_pin: microcontroller.Pin) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

spi_bus is type busio.SPI, the tell is how it can use the configure() method, and busio imports are common for the drivers.

"""
Initialize the programmer with an SPI port that will be used to
communicate with the chip. Make sure your SPI supports 'write_readinto'
Expand All @@ -100,7 +108,7 @@ def init(self, spi_bus, rst_pin):
self._rst.direction = Direction.OUTPUT
self._rst.value = True

def verify_sig(self, chip, verbose=False):
def verify_sig(self, chip: dict, verbose: bool = False) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

The typing for chip is correct and valid, but Python also allows for the use of generics, which allow you to specify the content of some types. In this case, we can specify the types of the keys and values of the dict.

To do this for Python 3.7+ (earliest Python supported by Blinka), we can import Dict from the typing module and use brackets to specify the key and value types. You can see an example of a dict that gets passed in here:

https://github.com/prcutler/Adafruit_CircuitPython_AVRprog/blob/daf67694dbbb4a0d4394d8e0c1439e261cdc03da/adafruit_avrprog.py#L60

All of the keys are str, so that will be that. For the values, it's a little trickier. We could use a Union to say it can take str, list, etc., but that's a bit of a clutter. The other option is to specify values as typing.Any which is kinda like saying "look, don't count on this being any specific type, it could be anything at all, so don't try to lint it or try to use it unless you know what it is". That's probably a good fit for this use case.

With all of that said, after import Dict and Any from typing, we can type it as:

Suggested change
def verify_sig(self, chip: dict, verbose: bool = False) -> bool:
def verify_sig(self, chip: Dict[str, Any], verbose: bool = False) -> bool:

I won't comment for every instance where this applies but you can do this for all of them!

"""
Verify that the chip is connected properly, responds to commands,
and has the correct signature. Returns True/False based on success
Expand All @@ -114,7 +122,7 @@ def verify_sig(self, chip, verbose=False):
return False
return True

def program_file(self, chip, file_name, verbose=False, verify=True):
def program_file(self, chip: dict, file_name, verbose=False, verify=True) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

Looking at one of the examples, it looks like filename is type str. Don't forget to add verbose and verify as well!

"""
Perform a chip erase and program from a file that
contains Intel HEX data. Returns true on verify-success, False on
Expand Down Expand Up @@ -182,7 +190,7 @@ def program_file(self, chip, file_name, verbose=False, verify=True):
self.end()
return True

def verify_file(self, chip, file_name, verbose=False):
def verify_file(self, chip: dict, file_name, verbose: bool = False) -> bool:
"""
Perform a chip full-flash verification from a file that
contains Intel HEX data. Returns True/False on success/fail.
Expand Down Expand Up @@ -229,7 +237,7 @@ def verify_file(self, chip, file_name, verbose=False):
self.end()
return True

def read_fuses(self, chip):
def read_fuses(self, chip: dict) -> list:
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this returns a tuple, which can also be annotated as a generic! In the case of tuples, the generic can specify the types on the tuple's contents. Since the tuple returned seems to be of length 4, all of which are int, we can import Tuple from typing and annotate it as:

Suggested change
def read_fuses(self, chip: dict) -> list:
def read_fuses(self, chip: Dict[str, Any]) -> Tuple[int, int, int, int]:

"""
Read the 4 fuses and return them in a list (low, high, ext, lock)
Each fuse is bitwise-&'s with the chip's fuse mask for simplicity
Expand All @@ -244,7 +252,7 @@ def read_fuses(self, chip):
return (low, high, ext, lock)

# pylint: disable=unused-argument,expression-not-assigned
def write_fuses(self, chip, low=None, high=None, ext=None, lock=None):
def write_fuses(self, chip: dict, low=None, high=None, ext=None, lock=None) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Seeing as how low, high, ext, and lock get passed to and used in the _transaction method, they need to be int. However, they're default is None which allows them to be short-circuited and skipped since None and [anything] else is always False, so [anything] never actually runs.

We use the poorly named Optional import from typing to specify that something can be None, so they types for all those is Optional[int].

"""
Write any of the 4 fuses. If the kwarg low/high/ext/lock is not
passed in or is None, that fuse is skipped
Expand All @@ -262,7 +270,7 @@ def write_fuses(self, chip, low=None, high=None, ext=None, lock=None):

# pylint: enable=unused-argument,expression-not-assigned

def verify_fuses(self, chip, low=None, high=None, ext=None, lock=None):
def verify_fuses(self, chip: dict, low=None, high=None, ext=None, lock=None) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the other arguments can be Optional[int] here as well.

"""
Verify the 4 fuses. If the kwarg low/high/ext/lock is not
passed in or is None, that fuse is not checked.
Expand All @@ -277,7 +285,7 @@ def verify_fuses(self, chip, low=None, high=None, ext=None, lock=None):
return False
return True

def erase_chip(self):
def erase_chip(self) -> None:
"""
Fully erases the chip.
"""
Expand All @@ -288,7 +296,7 @@ def erase_chip(self):

#################### Mid level

def begin(self, clock=_FAST_CLOCK):
def begin(self, clock: int = _FAST_CLOCK) -> None:
"""
Begin programming mode: pull reset pin low, initialize SPI, and
send the initialization command to get the AVR's attention.
Expand All @@ -299,14 +307,14 @@ def begin(self, clock=_FAST_CLOCK):
self._spi.configure(baudrate=clock)
self._transaction((0xAC, 0x53, 0, 0))

def end(self):
def end(self) -> None:
"""
End programming mode: SPI is released, and reset pin set high.
"""
self._spi.unlock()
self._rst.value = True

def read_signature(self):
def read_signature(self) -> list:
Copy link
Member

Choose a reason for hiding this comment

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

We can also define the contents of a list using generics. For lists, we only need to define what any given element can be. The return from _transaction() is bytearray, and an element of a bytearray is an int. Since we're appending only int to the list, we can import List from `typing and type the return here as:

Suggested change
def read_signature(self) -> list:
def read_signature(self) -> List[int]:

"""
Read and return the signature of the chip as two bytes in an array.
Requires calling begin() beforehand to put in programming mode.
Expand All @@ -317,7 +325,7 @@ def read_signature(self):
sig.append(self._transaction((0x30, 0, i, 0))[2])
return sig

def read(self, addr, read_buffer):
def read(self, addr: int, read_buffer: bytes) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Since we're writing things into what's provided into read_buffer, we'll need to use a type that can support writing using the indices. In this case, let's use bytearray, which supports that. Sometimes we can use an even less specific type, but given how it's used, I do think that only bytearray will work.

"""
Read a chunk of memory from address 'addr'. The amount read is the
same as the size of the bytearray 'read_buffer'. Data read is placed
Expand Down Expand Up @@ -345,7 +353,7 @@ def _flash_word(self, addr, low, high):
self._transaction((0x40, addr >> 8, addr, low))
self._transaction((0x48, addr >> 8, addr, high))

def _flash_page(self, page_buffer, page_addr, page_size):
def _flash_page(self, page_buffer: float, page_addr: int, page_size: int):
Copy link
Member

Choose a reason for hiding this comment

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

Looks like page_buffer needs to support access by indices and that individual elements are int. Looking at how it gets used in the internals, it looks like it expects a bytearray. You can also add the None return type!

page_addr //= 2 # address is by 'words' not bytes!
for i in range(page_size / 2): # page indexed by words, not bytes
lo_byte, hi_byte = page_buffer[2 * i : 2 * i + 2]
Expand Down Expand Up @@ -375,7 +383,7 @@ def _busy_wait(self):
pass


def read_hex_page(file_state, page_addr, page_size, page_buffer):
def read_hex_page(file_state: dict, page_addr: int, page_size: int, page_buffer: bytearray):
Copy link
Member

Choose a reason for hiding this comment

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

You can add the return type here as well, which appears to be bool.

# pylint: disable=too-many-branches
"""
Helper function that does the Intel Hex parsing. Takes in a dictionary
Expand Down