[refactoring] Moved chip-specific parameters into separate files

[rewolff]

HI,

I've created a preliminary version of the chip parameters in files stuff.

Please do not merge yet, this is not yet production-ready. In a project that I maintain, someone managed to mark this to github as "do not merge" but I don't know how to do that myself.

To use/test: Unpack the src/chips.zip file somewhere and use "st-util" from that directory. Other tools from the suite don't have the necessary "init" call added yet.

I have made an attempt to compare the old vs the new structures before returning the new one. This fails miserably as the pointers to the description strings will certainly be different, but for debug, it currently dumps both the old and the new struct.

I noticed that for the F072 that I currently have attached the options fields have not been filled out.

(Partially addresses #237)

[Nightwalker-87]

Please convert this into a draft as it fails on several build environments, especially on macOS and Windows.
It will also not be part of the upcoming release.

[rewolff]

I have pushed a fix for (some of?) the macos failures. All documentation says that this will propagate to the pull request, but it doesn't. Anybody know why?

[Nightwalker-87]

Check: https://travis-ci.org/github/stlink-org/stlink/builds/767310548 for the windows cross compile.

[Nightwalker-87]

It did - the build has just finished.

[rewolff]

Ok. Thanks! Got it!
Sorry for the first "not my problem": The first two or three I saw were indeed "not my problem" but that made me "stop looking".....

It seems I get past everything now that isn't ubuntu 16.04. Oh.... the macos check hasn't finished yet. Will check back later.

Next thing is to get someone to do something with a chip other than 'F072 that I can easily check....

[Nightwalker-87]

It should pass now on macOS.
The Ubuntu 16.04 build fails are caused by the package nettle-dev that seems to have disappeared from the repo.
There is no known dependency on this package from our side - it seems to be in use in the build environment itself.

[rewolff]

I have an old 16.04 machine. When I typed "sudo apt install nettle-dev" it just worked:
...
Get:3 http://nl.archive.ubuntu.com/ubuntu xenial-updates/main i386 nettle-dev i386 3.2-1ubuntu0.16.04.2 [960 kB]
...

[Nightwalker-87]

I know, I have also checked the official repo.
I believe it has to do with the virtual environment provided by GitHub Actions.
It suddenly stopped working unrelated to any specific change in this projects as I find it to be.
Here is an example log, if interested: https://github.com/stlink-org/stlink/runs/2364789728?check_suite_focus=true

[Ant-ON]

@rewolff I think that the lack of config files is a feature of the application. Everything works right away and from executable file. Such changes change the established architecture. Many problems appear. Many configurations have minor differences that are described only in the code.
I think need start with making the configuration files as additional data sources. The priority of the additional data may be higher than the basic one. It can also correct minor errors in the embedded data.

[rewolff]

Once the basis with these is there, "minor differences in the code" should be put into the configuration files. The code should then no longer:

if (chip == STM32H746) ...

but

// quirk36 first observed on STM32H746
if (chiphasquirk (chip, QUIRK36) ) ...

This means that as long as a newer chip doesn't have new quirks, an older utility can be made to work with chips that didn't exist when the st-link utility was built. Just plop the config file for the new chip into the right place.

If you want to have the default chips-config also embedded in the code, you're getting code duplication. A nightmare to maintain. And the users will get little feedback when they want to correct something minor but they edit the /etc/stlink/chips/myfile.chip instead of /usr/local/etc/stlink/chips/myfile.chip.

So you're suggesting that when my STLINK works on my ubuntu 20.04 system and my friend who hasn't upgraded still on 18.04 says it doesn't work for him, I can then say: Here, use my binary.

Well..... Let me just tell you that I'm no newbie to programming. After compiling new stlink utilities.... they didn't work as expected. Turns out there is a shared library that is installed when you do "make install". So it ISN"T just a binary at the moment. We are beyond the DOS days where a single executable is all you needed.

[Ant-ON]

@rewolff I just expressed my thoughts on the proposed changes.
ps I don't want to duplicate anything. In my view, the config file should be empty (with a commented out example of adding an entry).

[rewolff]

@Ant-ON , Thanks for your thoughts of course. If your opinion is shared by the (rest of?) the maintenance team here then It doesn't make sense to further invest time in this.

For a "one off" it is of course easier for me to just edit the source. So if this is not integrated, for myself I'd just edit in the "new chip" next time this pops up. But knowing it'll be easier for myself to add a new chip in the future in addition with knowing that others will benefit as well is what makes it worth my trouble to finish this.

But if my suggestion is voted out, then next time, my code will be out-of-date with stock stlink and next-to-useless. So not even I would be able to benefit from the work I put in. And nobody else would benefit from my improvements either. So not worth investing more time in this if that's what's going to happen.

If the powers that be refuse to hint "good idea, I think we'll include it once it is done" or "show a completed patch, then we'll decide" then I'm afraid I won't put in any more time. Too many times already I've put in work to clean up such a patch only to be met in the end with "sorry, I don't like the principle". Fine, it's your project, but tell me before I put in the time to "make it ready for inclusion".

---

I really think that it is better not to have these chip details baked into the program (even if you can override them with a config file). Even with your "and an empty configuration file with a template". Just providing all config files with the program means that you give the user a bunch of working examples, not just one template. Of course, we could keep the "export builtin database", but I still think that'll be harder-to-use for most people trying to add say the latest STM32F4xx. They should in my book be able to look at the other files for STM32F4yy and use the one they think matches most. People won't think of dumping the internal database if you give them just a template to work with.

[Nightwalker-87]

I would at least prefer to have all chip-specific parameters, moved out from the source code into separate files for each STM32 MCU family. However I can't give any precise assessment on how this could be implemented at it's best. Maybe moving these out is an intermediate approach which can be taken before refining the implementation later on.

[rewolff]

Thanks for the encouragement @Nightwalker-87 .

[Nightwalker-87]

In general I'd further prefer to continue to modularize the toolset over time.
common.c for example appears very unstructured and contains loads of code that should be simplified as well as moved to the respective modules. The reason for that is that it has it's origin in the very early days of this project AFAIK.
However this goes far beyond the scope of this ticket - I just wanted to point that out as it is related.

[rewolff]

:-) Yeah, I'm not touching that with a long stick. :-)

[rewolff]

Updated:
Separated the chip.zip file into a directory in the sources.
Wrestled with CMAKE to install the chip files in /usr/local/etc or /etc as appropriate.
Wrestled with CMAKE to make the used directory available inside the program.
some debug "processing file ... " removed.
Added the init to the other programs from the suite so that should work now too.

Left the old vs new debug info for now once the chipid is looked up.

TODO: split the bitfields in "flags" into separate human readable strings (for input and output).

RFC: I propose using "atoi" for the ascii-to-int conversion. This changes the file format in that 0x is required before all the numbers. Pro: Documents that they are in hex "in place". con: Changes the files. Makes them bigger, No decimal numbers are used anyway. Proposal: just do it.

[Ant-ON]

@rewolff application data is usually stored in /usr/share/APP_NAME

[rewolff]

IMHO, such config files are often stored in /etc/ ...
Now it's debatable if these are config files or "application data".
More votes on the matter are welcome.

[Nightwalker-87]

I may take care of the naming part then.

Regarding your proposal: I don't really fancy to store data in home directories.
Instead user-specific, customised changes should be applied locally in the source code (e.g. within an IDE).
However the rest of the approach seems reasonable to me.

[Ant-ON]

@Nightwalker-87 I prefer not to participate in decision-making on this PR. It is better to listen to the opinion of other contributors here. My opinion here can ruin everything. Sorry.

ps Don't forget about Windows and Mac OS support.
pss Use $HOME/.stlink/... (same as $HOME/.config/...) is common practice for custom settings in linux.
psss I am using the stlink as standalone library.

[Nightwalker-87]

@Ant-ON That's OK you can do, I don't mind. An open discussion should be most preferable.
I only expressed my personal thoughts on this.

[Nightwalker-87]

As far as I can see we currently have such duplication as mentioned earlier. I understand this as a pre-cautious attempt by @rewolff in order not to break anything.

@Ant-ON Can you precise your concerns? Where are the spots that need extra attention?
If possible we should continue with the structural transition.
I am currently going through the chip ID config files to align the naming scheme.

[Nightwalker-87]

The naming scheme is now aligned and I am waiting for further response @Ant-ON @rewolff.

[rewolff]

I'll try to work on this the coming weekend.

[rewolff]

This is what I was able to do. Little testing, but the files parse again... Next up: implementing a list of directories to scan.

[Nightwalker-87]

@rewolff I'll merge this into the branch testing now, as there is another PR from @Ant-ON also attempting to address the naming scheme and the amount of changes in this PR has become quite large already.
Nevertheless I am well aware of that this topic here is not yet complete. We still have:

• directory scanning
• detailed comparison of old and new configuration
• testing of some MCU (which are available) while having old configuration settings commented out
• final clean-up of the old configuration

We should address these points in a subsequent PR to the testing branch.