cvra / can-bootloader

The bootloader used to flash our CAN-connected boards
BSD 2-Clause "Simplified" License
144 stars 51 forks source link

Flasher tool #42

Closed antoinealb closed 9 years ago

antoinealb commented 9 years ago

This PR implements the flashing tool for the bootloader. It is almost complete and should be testable with real hardware soon. I used the code from @nuft as a start, and added tests. Some corner cases might not be covered yet and maybe some are only observable using real hardware.

1.0 feels so close now :+1:

antoinealb commented 9 years ago

The last missing stuff in my opinion is waiting for boards to be ready (related: #27).

@nuft Could you this tool quickly in real life ? Simply adding a few time.sleep() to flash_binary and crc_regioncould be enough to make it work.

msplr commented 9 years ago

Great! Already some remarks for the read_can_datagram() function:

However, it should already work as it is now. I'll try to test it today. I'll review more thoroughly later.

antoinealb commented 9 years ago

Well since we only have 3 month left before the Belgian contest. If it works well enough, ship it. We will fix it when / if problems arise.

msplr commented 9 years ago

So I came to run it with a prototype bootloader, and it worked! I had to make some little changes:

[...]
Verifying firmware...
Traceback (most recent call last):
  File "bootloader_flash.py", line 169, in <module>
    main()
  File "bootloader_flash.py", line 154, in main
    valid_nodes_set = set(check_binary(serial_port, binary, args.base_address, args.ids))
  File "bootloader_flash.py", line 88, in check_binary
    crc = crc_region(fdesc, base_address, len(binary), node)
  File "bootloader_flash.py", line 128, in crc_region
    answer = read_can_datagram(fdesc)
  File "bootloader_flash.py", line 116, in read_can_datagram
    frame = can_bridge.decode_frame(frame)
  File "[...]/can-bootloader/client/can_bridge.py", line 36, in decode_frame
    unpacker.feed(data)
  File "_unpacker.pyx", line 276, in msgpack._unpacker.Unpacker.feed (msgpack/_unpacker.cpp:276)
TypeError: 'NoneType' does not support the buffer interface
antoinealb commented 9 years ago

I have fixed the two errors. I will add the application run command. For the delays, I don't know: is it much of a problem ? We can still add it later.

msplr commented 9 years ago

I'm ok with merging now, since it is mostly working. However, before 1.0 I want these issues fixed:

antoinealb commented 9 years ago

Where did you add the delays ? Also I am not sure what you mean by proper CAN datagram handling.

antoinealb commented 9 years ago

So once #43 is merged, I will be able to fix timing properly.

msplr commented 9 years ago

For testing, I just added a delay of about 0.3 seconds to write_command(). With the CAN datagram handling I meant mostly the issues mentioned in my first reply. Most important of this: distinguish between different source IDs to allow multicast polling, as proposed by @Stapelzeiger in #27.

antoinealb commented 9 years ago

Well I am not sure if multicast polling is critical. I would say we already finished the critical part of the project, don't you think so ?

msplr commented 9 years ago

Yes, It doesn't need to be implemented right now. Though it doesn't harm if we lay things out now for later improvement. For me, we can merge.