Skip to content

Adapt to new modm-devices pinout format #8

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

Closed
wants to merge 1 commit into from

Conversation

salkinium
Copy link
Contributor

@salkinium salkinium commented Aug 29, 2020

The pinout information is now stored in a separate list instead of being
part of the GPIO table. To ensure that this format is useful for you, I've refactored the data intake to use the new format.

I've merged the modm-devices pull request modm-io/modm-devices#46 and published [email protected].

cc @tgree.

@salkinium salkinium force-pushed the feature/modm_device_update branch 3 times, most recently from 1818f04 to c1c9240 Compare August 30, 2020 09:32
@salkinium salkinium marked this pull request as ready for review August 30, 2020 09:33
The pinout information is now stored in a separate list instead of being
part of the GPIO table.
@salkinium salkinium force-pushed the feature/modm_device_update branch from c1c9240 to 8505bbd Compare August 30, 2020 10:55
@salkinium
Copy link
Contributor Author

I also added the data to filter out remapped pins, and tested this with the STM32G0 patch.

@tgree
Copy link
Owner

tgree commented Sep 20, 2020

Hey @salkinium!

Sorry for the late reply, it has been a very busy summer for me! I merged this pull request from a separate branch after I reworked it a bit - it includes your commit and then one after. The main problem with the pull request was on chips like the stm32h745igk6. These chips have separate physical pins named "PC2" and "PC2_C" (and a few others with the same convention). The "_C" pins are dedicated analog pins that go straight into the ADC, bypassing things like the MODER switch and in theory should allow for a cleaner signal to the ADC (I have yet to experiment with these but our latest design includes it). The easiest place to see what I'm talking about is in RM0399 Rev 3, page 577, fig 82 which I've attached here:

Screen Shot 2020-09-20 at 1 00 19 AM

The way your pull request was parsing the short pin name we'd end up with two pins in stm_layout's UI labelled "PC2" instead of one "PC2" and another "PC2_C".

The other thing I've noticed is that I think modm-devices is parsing the XML for PC2 and PC2_C incorrectly. The chip manual STM32H745xI/G Rev 1 shows that PC2 and PC2_C should have a bunch of alternate functions:

Screen Shot 2020-09-20 at 1 02 41 AM

However, the parsed XML returns no alternate functions for instance for chip "stm32h745igk6". The generated XML seems a bit confused - from stm32h7-45_55.xml I see:

      <gpio port="c" pin="2">
        <signal device-package="h|k" driver="adc" instance="1" name="inn11"/>
        <signal device-package="h|k" driver="adc" instance="1" name="inp12"/>
        <signal device-package="h|k" driver="adc" instance="2" name="inn11"/>
        <signal device-package="h|k" driver="adc" instance="2" name="inp12"/>
        <signal device-package="t" driver="adc" instance="3" name="inn1"/>
        <signal device-package="h|k" driver="adc" instance="3" name="inn11"/>
        <signal device-package="t" driver="adc" instance="3" name="inp0"/>
        <signal device-package="h|k" driver="adc" instance="3" name="inp12"/>
        <signal af="3" driver="dfsdm" instance="1" name="ckin1"/>
        <signal af="5" driver="i2s" instance="2" name="sdi"/>
        <signal af="5" driver="spi" instance="2" name="miso"/>
        <signal af="6" driver="dfsdm" instance="1" name="ckout"/>
        <signal af="10" driver="usb_otg_hs" name="ulpi_dir"/>
        <signal af="11" driver="eth" name="txd2"/>
        <signal af="12" driver="fmc" name="sdne0"/>
      </gpio>
      <gpio device-package="h|k" port="c" pin="2">
        <signal driver="adc" instance="3" name="inn1"/>
        <signal driver="adc" instance="3" name="inp0"/>
      </gpio>

It looks like the second clause with the device-package="h|k" override is clobbering the first clause even though the first clause already has "h|k"-specific signal tags. From the chip documentation, it looks like PC2 should have ADC3_INN11/P12 (among others) while PC2_C should only have ADC_INN1/P0. I suspect that the way modm-devices is currently designed that it may have trouble with this, but I haven't looked at the ST source XML; if modm-devices is just discarding a "_C" suffix then it may be possible to expect the class to differentiate the two pins, maybe by adding a variant attribute or something like that. Since the PC2 and PC2_C functions are actually different, that implies to me that probably we need to treat them as separate logical things even though they are very closely related.

If you would like, I would be happy to open a modm-devices issue with this instead of leaving it buried in this PR!

@tgree
Copy link
Owner

tgree commented Sep 20, 2020

Actually, I just refactored stm_layout to now depend on the pypi.org version of modm-devices instead of cloning it manually (as a first step to making it a pip-installable tool) and it is now giving me the alternate functions, so I think modm-devices has changed since your feature/pinout branch that I was pointed at. This is the generated XML in the pypi.org version:

      <gpio port="c" pin="2">
        <signal device-package="h|k" driver="adc" instance="1" name="inn11"/>
        <signal device-package="h|k" driver="adc" instance="1" name="inp12"/>
        <signal device-package="h|k" driver="adc" instance="2" name="inn11"/>
        <signal device-package="h|k" driver="adc" instance="2" name="inp12"/>
        <signal device-package="t" driver="adc" instance="3" name="inn1"/>
        <signal device-package="h|k" driver="adc" instance="3" name="inn11"/>
        <signal device-package="t" driver="adc" instance="3" name="inp0"/>
        <signal device-package="h|k" driver="adc" instance="3" name="inp12"/>
        <signal af="3" driver="dfsdm" instance="1" name="ckin1"/>
        <signal af="5" driver="i2s" instance="2" name="sdi"/>
        <signal af="5" driver="spi" instance="2" name="miso"/>
        <signal af="6" driver="dfsdm" instance="1" name="ckout"/>
        <signal af="10" driver="usb_otg_hs" name="ulpi_dir"/>
        <signal af="11" driver="eth" name="txd2"/>
        <signal af="12" driver="fmc" name="sdne0"/>
      </gpio>

This is a great improvement! Unfortunately, it still doesn't quite match the screenshot of PC2_C I attached above; PC2 is perfect, however.

It's actually kind of ambiguous; depending on the state of the SYSCFG_PMCR.PxySO bit controlling the analog switch, either PC2 or PC2_C could gain all of the ADC alternate and additional functions from the other physical pin. The nasty bit on figure 82 is that the two "To ADC" arrows go to different ADC inputs... :/

@tgree
Copy link
Owner

tgree commented Sep 20, 2020

FWIW, I made stm_layout pip-installable. You can see the PC2 and PC2_C issue most easily with:

pip3 install stm_layout
stm_layout -c stm32h745igk6

@salkinium
Copy link
Contributor Author

I think modm-devices has changed since your feature/pinout branch that I was pointed at.

Yes, we implemented the remap functionality in the modm C++ API and found some issues with the data that was then fixed.
However, the STM32H7 does not have a modm port yet, so we're not finding the issues that you pointed out, since we don't access the data at all.

I'm working on unit tests that check certain properties of the data itself, so that exactly these problems become testable for regressions. But I got stuck with other things: modm-io/modm-devices#35

It's actually kind of ambiguous;

Ah, my favorite kind of data ;-P

I made stm_layout pip-installable.

Fantastic! One step closer to ridding the planet of the CubeMX plague!1!! *shakes fist in direction of ST HQ*

@salkinium
Copy link
Contributor Author

It would be good if you could open up an issue in modm-devices, otherwise I'll have forgotten about this in a few weeks.

@salkinium salkinium closed this Sep 20, 2020
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