Bring revC2 firmware to the same level as revC1 #242

electroniceel · 2020-12-05T23:50:14Z

These commits bring the firmware for revC2 to the same level as revC1.
This means you can read voltages, set alert levels, read alert levels, read alert status. Also alerts are properly handeled and the Vio switched off. After an alert was cleared, you can re-arm the alert.

This does not yet make use of the advanced features of the INA233, e.g. current measurement, current alerts or the enhanced voltage range of up to 36V. This will be developed at a later stage.

These changes do not alter the control request interface, so no API level change required.

This is the first part of #219

whitequark · 2020-12-27T07:04:36Z

firmware/main.c


  // TODO: handle i2c comms errors of above calls
+
+  iobuf_set_voltage(mask, &millivolts);


Should this only happen on non-C2? If so the control flow here could be simplified.

this should happen on all revs. And on revC2 and later it must happen before iobuf_clear_alert_ina233() is called, because otherwise the already disabled voltage would be re-enabled for a short moment.

But if you prefer that, I could create two bigger blocks (revC1 and older, and revC2 and later), each with their own call to iobuf_set_voltage(), just to make the code easier to read and have the conditionals only once.

If this should happen on all revs the existing control flow is OK. I think I will (once all of your changes are merged) go through all of the modified code and add/expand comments everywhere that I find confusing, with your input of course.

I think I will start by adding some more comments while addressing your points from this review cycle. Then you can go from there and add some more if necessary.

whitequark · 2020-12-27T07:06:12Z

firmware/util.c

@@ -2,6 +2,18 @@
 #include <fx2i2c.h>
 #include "glasgow.h"

+bool i2c_direct_read(uint8_t addr, __pdata uint8_t *value, uint8_t length) {


This function doesn't seem to be used anywhere?

ah, right. I accidently left it in there.

It is needed to clear the ALERT line the proper way, once the address clash is resolved. So it will have to be added again later. Do you want me to remove it now or should we keep it?

I prefer to only have stuff in code that is actually used, otherwise the code gets more confusing to readers, and such snippets can get lost if later we decide to implement something in a different way. Also, sdcc doesn't eliminate dead code (other than entire modules during linking), so this function actually has non-zero runtime cost.

no dead code elimination? ugh, that is bad. will remove it for now then.

Sdcc only has peephole optimizations. Everything else goes into the assemby (and so, the binary) the way you wrote it.

whitequark · 2020-12-27T07:07:23Z

firmware/adc_ina233.c

 };

 struct buffer_desc {
  uint8_t selector;
+  __xdata uint8_t* status_cache_ptr;


Can you explain how the status cache works? I don't understand why it's necessary.

The status_cache is stored per INA233, that means per channel. Once someone pulled the ALERT line, each INA233 is queried for it's status register. That is the one that contains if an alert was triggered and which one. If an alert was found, it is stored in the status_cache (function iobuf_poll_alert_ina233()).

To make the INA233 release the ALERT line, I have to reset it with the RESTORE_DEFAULT_ALL command (iobuf_clear_alert_ina233()). This is the quirk that is necessary because of the i2c address clash on revC2. Sending the reset also clears the status register of course. So a later USB_REQ_POLL_ALERT wouldn't be able to know that there was an alert and on which port and so on. To keep this info around, the status_cache is needed.

It is cleared once a USB_REQ_POLL_ALERT is sent. This is the same moment the status register of the adc081c is cleared on previous revs, so it implements the same functionality for the user.

Do we have to keep this forever? Can we make some instructions for rejigging existing revC2's to be more like revC3's and then kill the firmware workaround with fire?

On first glance it looks like the change required would be just one bodge wire, connecting one previously unconnected pin of the dac (SOT-23-6) to 3v3. So while it still is rework, it is not unreasonably fine pitch. If we require this bodge, then we could kill the whole cache code.

If a unmodified revC2 is used without the cache code, you can't recover from a alert situation without resetting Glasgow.

whitequark

Looks fairly reasonable besides the inline comments.

whitequark · 2020-12-27T07:36:17Z

firmware/main.c

@@ -791,6 +791,10 @@ void handle_pending_usb_setup() {
 volatile bool pending_alert;

 void isr_IE0() __interrupt(_INT_IE0) {
+  // INT_IE0 is level triggered, the ~ALERT line is continously pulled low by the ADC
+  // So disable this irq unil we have fully handled it, otherwise it permanently triggers
+  EX0 = false;


We could probably reuse EX0 as a pending alert indicator, to avoid having two pieces of state that can go out of sync. (If we do, we should alias it under a name like armed_alert and nicely document it.)

With "alias" you mean a define?

Otherwise I like this idea and will implement it.

I mean declare it the same way fx2regs.h defines the EX0 symbol in first place. (You can define as many symbols pointing to the same SFR bit as you want, sdcc doesn't care.) Though I suppose a define would also work; I have no real preference between these options.

whitequark · 2020-12-27T07:36:52Z

I've cherry-picked the first three commits that are unobjectionable and nicely improve our level of support for revC2.

electroniceel · 2020-12-28T19:25:10Z

The mentioned issues should be resolved now

whitequark · 2020-12-29T02:26:55Z

Cherry-picked the ~ALERT commit. Regarding the cache, do you think we can poll and clear the alert in one operation while recording what happened, and then communicating that to the USB host separately? Essentially, making the "cache" logic common for all revisions and moving it to main.c. This is just an idea and if it doesn't make much sense, please say so.

electroniceel · 2020-12-29T10:24:54Z

Regarding the cache, do you think we can poll and clear the alert in one operation while recording what happened,

Poll and clear in one operation won't work on revC2: when you clear the ~ALERT, you re-enable the Vio that was previously disabled by the hardwired logic. You have to disable the voltage regulator first. And for this you have to know which one, that means polling the status.

So poll and clear have to stay separate.

Essentially, making the "cache" logic common for all revisions and moving it to main.c.

I don't think that the cache for the alert status is that bad, so it could be made the default. IIRC, your main argument against the cache is that it could go out of sync with the hardware. But if we reset the adcs thoroughly during init in main (like I do now with the INA233), I don't see how that could happen.

If we move it to main, we'd have to translate it from the adc specific bits to a generic format though. Also main doesn't know about the ports yet, it just deals with port-masks. So we'd have to introduce a small array for the ports to main.

whitequark · 2020-12-29T10:26:53Z

OK, I think this is probably fine. Can you rebase? Then I'll give it another look and probably merge.

electroniceel · 2021-01-10T15:27:13Z

I now think the cache and reset logic used in this PR should stay and be used for revC3 too, see #221 (comment) for details

Implements a workaround for an i2c address clash of the SMBus Alert Request Address (required to clear the ~ALERT line) and the DAC A.

whitequark reviewed Dec 27, 2020

View reviewed changes

electroniceel force-pushed the wip-revC2-firmware branch from e472c43 to c48983d Compare December 28, 2020 19:24

electroniceel mentioned this pull request Dec 28, 2020

firmware: switch the DAC off too when switching off Vio #243

Merged

electroniceel force-pushed the wip-revC2-firmware branch from c48983d to 04dbe76 Compare December 29, 2020 10:33

Base automatically changed from master to main February 27, 2021 09:59

firmware: handle voltage alerts on revC2

b9e641a

Implements a workaround for an i2c address clash of the SMBus Alert Request Address (required to clear the ~ALERT line) and the DAC A.

esden force-pushed the wip-revC2-firmware branch from 04dbe76 to b9e641a Compare December 3, 2022 05:51

esden mentioned this pull request Dec 13, 2022

Add support for the rev C3 hardware #295

Merged

whitequark approved these changes Dec 13, 2022

View reviewed changes

whitequark merged commit a35d8c4 into main Dec 13, 2022

whitequark deleted the wip-revC2-firmware branch December 13, 2022 04:28

attie mentioned this pull request Feb 23, 2023

Firmware support for INA233 on revC2 #219

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring revC2 firmware to the same level as revC1 #242

Bring revC2 firmware to the same level as revC1 #242

electroniceel commented Dec 5, 2020 •

edited

whitequark Dec 27, 2020

electroniceel Dec 27, 2020

whitequark Dec 28, 2020 •

edited

electroniceel Dec 28, 2020

whitequark Dec 27, 2020

electroniceel Dec 27, 2020

whitequark Dec 28, 2020 •

edited

electroniceel Dec 28, 2020

whitequark Dec 28, 2020

whitequark Dec 27, 2020

electroniceel Dec 27, 2020

whitequark Dec 28, 2020

electroniceel Dec 28, 2020

whitequark left a comment

whitequark Dec 27, 2020

electroniceel Dec 27, 2020

whitequark Dec 28, 2020 •

edited

whitequark commented Dec 27, 2020

electroniceel commented Dec 28, 2020

whitequark commented Dec 29, 2020

electroniceel commented Dec 29, 2020

whitequark commented Dec 29, 2020

electroniceel commented Jan 10, 2021


		// TODO: handle i2c comms errors of above calls

		iobuf_set_voltage(mask, &millivolts);

Bring revC2 firmware to the same level as revC1 #242

Bring revC2 firmware to the same level as revC1 #242

Conversation

electroniceel commented Dec 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitequark Dec 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitequark Dec 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitequark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitequark Dec 28, 2020 • edited

Choose a reason for hiding this comment

whitequark commented Dec 27, 2020

electroniceel commented Dec 28, 2020

whitequark commented Dec 29, 2020

electroniceel commented Dec 29, 2020

whitequark commented Dec 29, 2020

electroniceel commented Jan 10, 2021

electroniceel commented Dec 5, 2020 •

edited

whitequark Dec 28, 2020 •

edited

whitequark Dec 28, 2020 •

edited

whitequark Dec 28, 2020 •

edited