Redesign/reimplement firmware and add revD support - #1181
Conversation
6f4b75c to
13aa799
Compare
af4df3e to
b20d903
Compare
whitequark
changed the title
43c4457 to
b374255
Compare
q3k
left a comment
q3k
left a comment
There was a problem hiding this comment.
The 8051 code seems to do what it should do. But that's a given, because it has already been tested to work.
But that's only part of what a code review is for... For me it's also about having the code be understood and maintained by more than one person. So I approached it as an exercise in me getting into the code and trying to understand it pretty much fully, and this turned out to be more of a readability review than a correctness review.
Overall I do believe that it would really benefit from a quite deep documentation pass. This code is on an intersection between two hellish topics (8051 SDCC code and USB) and even the smallest amount of documentation is immensely helpful. It took me a while to get through all of this because I basically had to build a mental model of the codebase from scratch with almost nothing helping me along the way. There's a lot of concepts that jump out on a first read (ports? endpoints? configuration? fpga configuration? nvm? i2c? smbus?) and it's difficult to read the code subsystem-by-subsystem because of how intertwined everything ends up being. Some handholding of the concepts being clearly explained when first encountered (ie. at the top of files, eg. header files) would have really helped steer me from having to guess and test my assumptions vs. just being able to get on with reading things.
I've left a bunch of comments requesting such docs, which are just a reflection of a small subset of thoughts I've had pop into my head while reviewing the code at first and having trouble understanding what's going on. These could be a good jumping off point to add critical documentation points.
But more broadly I think that:
- All 'public' functions in headers need a documentation string, and a quite verbose one at that. Assume basic famililarity with how a Glasgow works, but not necessarily how the FIFO setup works, how endpoints get reconfigured as the device is used, etc.
- All 'public' structures/enums/types in headers need a documentation string.
- Documentation should clearly state when some is used and by what, are values/types some parts of some fixed protocol or are they just some local decisions/defines.
- Extra care should be put in to document 'hidden' argument variables that would usually be passed explicitly but end up being global variables due to 8051/SDCC constraints.
| uint32_t bitstream_size; | ||
| char bitstream_id[CONFIG_SIZE_BITSTREAM_ID]; | ||
| uint32_t unused; | ||
| uint16_t voltage_limit[4]; |
There was a problem hiding this comment.
This would be a good place to document what voltage limits are.
| return smbus_run(get_vsupply_revabc_seq, dac_addr_revabc[chan]); | ||
| } | ||
|
|
||
| // ===== revABC01 ADC081C ========================================================================= |
There was a problem hiding this comment.
I would probably split this back out into multiple files per ADC/generation?
There was a problem hiding this comment.
I did it this way because there are no clear generations. We have a smooth transition from revA to revD that replaces bits piecemeal without ever having a clean break. So there is a clear case to be made for a ton of source files, or for a single file. I figured a single file makes more sense here, considering how it's all interdependent anyway.
(I did not originate the per-ADC split, it was done by someone else who implemented the INA233 support. That said, it made more sense with revC.)
|
Thanks for the review! I essentially agree with your conclusions. I'll reply to some of the comments that relate to technical constraints first, then update the firmware sources to address review (which will be almost exclusively documentation work). |
ld-cd
left a comment
ld-cd
left a comment
There was a problem hiding this comment.
still need to look through the python changes
f540b84 to
69b8b0d
Compare
whitequark
changed the title
This is currently not well surfaced (in particular the types are not
updated for this case), but it's a useful escape hatch e.g. to talk
to the NAFE using the SPI controller applet:
# ...
afe_adc = assembly._platform.request("afe_adc", dir={
"reset": "o",
"cs": "-",
"clk": "-",
"copi": "-",
"cipo": "-",
"int": "i",
"sync": "o",
"drdy": "i",
})
# ...
spi_iface = SPIControllerInterface(logger, assembly,
cs=~afe_adc.cs, sck=afe_adc.clk,
copi=afe_adc.copi, cipo=afe_adc.cipo,
mode=spi.Mode.from_cpol_cpha(cpol=0, cpha=1)
)
# ...
This error is reported by libusb if you stall a Set Interface request.
cc37db0 to
c5619ec
Compare
ld-cd
left a comment
ld-cd
left a comment
There was a problem hiding this comment.
Still got some more to work through
ld-cd
left a comment
ld-cd
left a comment
There was a problem hiding this comment.
Thats everything I was worried about resolved
The new FX2 firmware has complete feature parity with the old FX2
firmware, with the following major changes:
- reimplement board/port/FPGA management USB protocol to be network
transparent
- reimplement handling of USB endpoint buffering to significantly
increase throughput on Windows and open the door for e.g. all-OUT or
all-IN configurations
- reimplement port management commands to take a mask and four distinct
values (documented in `firmware/fx2/glasgow_mgmt.h`)
- add support for ECP5 SRAM bitstream loading
- add support for using ports `C`, `D` on revD0
- extend I/O voltage range to 1.2-5.5 V on revD0 (from 1.8-5.0 V on
revABC)
- implement a more robust fault system:
- faults are distinguished by the port they occurred on, and on their
type (Isupply over-current, Vsense overvoltage, Vsense undervoltage)
- faults can be cleared in a fine-grained manner
- unsolicited fault reports are sent by the firmware
- unsolicited fault reports are logged by the software
- add support for FPGA reporting interrupts via I2C and the FX2
- uses the same mechanism as supply fault reporting
- fully implemented and tested in FX2 firmware
- not yet integrated in software from either FPGA or host side
- implement supply disconnection on over-current on revC23D
- add CLI for current limiting: `glasgow voltage AB 3.3 0.125` (3.3 V,
125 mA)
- implement `glasgow flash --advertise-webusb` to get rid of
the notification
- disable Vsense range alert on revABC01 due to suspected ADC silicon
erratum (see `firmware/fx2/port.c`)
The following revD-only features are not implemented for the time being:
- STM32 bootloader in FX2
- NAFE bridge commands via the STM32
- ECP5 non-volatile bitstream programming/loading and STM32-based
strapping
The full functionality is implemented for both iCE40 and ECP5.
576fe83 to
234f5fe
Compare
See GlasgowEmbedded#1203. This is a temporary solution in lieu of something better
234f5fe to
b2305d7
Compare
3 participants
The new FX2 firmware has complete feature parity with the old FX2 firmware,
with the following major changes:
throughput on Windows and open the door for e.g. all-OUT/all-IN configurations
(documented in
firmware/fx2/glasgow_mgmt.h)C,Don revD0(Isupply over-current, Vsense over-voltage, Vsense under-voltage)
glasgow voltage AB 3.3 0.125(3.3 V, 125 mA)glasgow flash --advertise-webusbto get rid of the notification(see
firmware/fx2/port.c)The following revD-only features are not implemented for the time being:
To do:
To test:
`glasgow run benchmark` on Windows