DISTORTEC / distortos

object-oriented C++ RTOS for microcontrollers
https://distortos.org/
Mozilla Public License 2.0
430 stars 66 forks source link

Pins configuration for the supported evaluation boards #25

Closed CezaryGapinski closed 4 months ago

CezaryGapinski commented 7 years ago

Distortos supports the evaluation boards, but configurations are limited only to support leds or buttons. Often boards have extension headers, that can be used for many kind of custom usages. Where is a good place to put the user configs for pins? Do I need to put my config to main() function (but os is already running)? Low level Initialization for board is called before main() function and I think this is the best place to put other settings. I need to create my own full customization for eval board and own lowLevelInitialization() function?

FreddieChopin commented 7 years ago

Hi!

Generally configuration of various other peripherals (other than LEDs and buttons) is an issue I did not solve yet. Initially I was planning to have them configurable via Kconfig menus, but the thing that stopped me is that there's no easy way to include configuration of pins in such method. It would be simple if all chips within a family had - for example - USART1_TX on the same set of pins, but in reality is not that simple.

So for now I planned to add configuration of fixed board interfaces directly to board - just like with LEDs and buttons. For example on Nucleo boards one of the UARTs is always connected to the ST-Link's virtual COM port and I planned to add that. As for all other interfaces, which are not "fixed" (they are not used, there is nothing connected) I was planning to just add them too with one set of pins, selected arbitrarily (or maybe add a configuration to Kconfig if that would not be too complex?). This way you could have distortos/board/serialPorts.hpp and distortos/board/spiMasters.hpp and so on. Whether it's a good idea - I'm not sure yet, that's why I'm still thinking about it. I'm not entirely sure doing it would be worth the trouble, but I didn't have much time to think about it yet.

If you want to use distortos in a "normal" project - which you don't plan to make highly configurable to work on multiple boards - you can actually just ignore this whole issue with boards and be done with that. distortos doesn't need the board namespace of distortos/board/ include path. In a project like that you can initialize pins directly in main() or you can provide your own board::lowLevelInitialization() and do it there. The difference is - as you noticed - that the latter is executed before all global constructors, but in a typical project this doesn't make much difference anyway. You just have to make sure to configure the pins before you USE the peripheral, not before you construct its object. So you may as well configure the pins right before a call to serialPort1.open() or mySpiDevice.open() - no problem in that. The function used to configure pins is thread safe, so it can be executed when the scheduler is already running. But you may want to do all RCC configuration in board::lowLevelInitialization() or at least within a critical section (with an architecture::InterruptMaskingLock object in scope).

In my own projects I just create board::lowLevelInitialization() function where I configure RCC and all pins which need alternate function - you don't have to configure the pins which are used as normal outputs (like LEDs) or inputs (like buttons) here, but you should enable them in RCC here. If this is a board designed for a specific project - so nothing "generic" or for general purpose evaluation - there's no need to create anything more - like board's Kconfig files or board includes. You may do it, but this won't make things simpler or better.

I know this answer is very vague, but your question is also not very specific (; . If you don't understand anything I wrote above - ask here (or on mailing list). If you have some ideas - don't hesitate to share them here (or on mailing list), as you probably noticed, this project is in its early stage of development (but nevertheless perfectly usable in commercial projects (; ), so not all decisions are already made, and those that are can be changed if a better option is proposed. Obviously if you have more questions about this topic or you can make your question more specific - ask!

CezaryGapinski commented 7 years ago

I know this is a big issue. I think it is similar issue like in linux prior to the introduction of the Device Tree. Would be nice to have something to simply describe configuration like here (for spi): https://github.com/beagleboard/bb.org-overlays/blob/master/src/arm/BB-SPIDEV0-00A0.dts But I think is hard to implement something similar on KConfig menus. Have you thought to use something different like KConfig to generate board configuration files (source files or headers) based on device description files?

I'm not against the creation of fixed board interfaces directly to board. It's sounds good and simply. Based on linux boards like beaglebone, rasppery, there is also configuration for specific board described with pins and theirs functions. If someone want to create some customization is always able to do this with, like in own board::lowLevelInitialization(). If I understand correctly you want to add some sets of pins for board, but how do you want to "abstract" for this configuration pins? There would be sets of pins groups like: board/serialPorts.hpp, board/spiMaster.hpp, board/leds.hpp, board/buttons.hpp? What if spi or rs485 communication device needs gpio pin for chip select. Do you plan to add board/gpioInputPins.hpp and board/gpioOutputPins.hpp or it would be included in serialPorts or spiMaster "objects"? I created my own spiPins with chip select pin, but I think it is a bad idea and it's better to create separated files with gpioInput and Output pins like for buttons and leds.

I know my question was not very specific, I'm just asking :). For a long time I've stuck in C code for embedded systems, so your project is something new on this field (like for many others embedded developers, but thanks to popularity of new cpp standards this can change soon enough).

FreddieChopin commented 7 years ago

But I think is hard to implement something similar on KConfig menus.

Yes, the syntax is a little bit limiting and doing something as big as pin configuration would probably soon became unmanageable. But the real question here is whether having such pin configuration in Kconfig is really necessary (in my opinion - no) and whether it would bring enough value to justify the burden of maintaining it (again, I don't think so now).

I think it is similar issue like in linux prior to the introduction of the Device Tree. Would be nice to have something to simply describe configuration like here (for spi): https://github.com/beagleboard/bb.org-overlays/blob/master/src/arm/BB-SPIDEV0-00A0.dts [...] Have you thought to use something different like KConfig to generate board configuration files (source files or headers) based on device description files?

Hmm... That's an interesting idea! In the repository we would only have things that are fixed (like LEDs, buttons, UART connected to ST-Link on Discovery/Nucleo boards) and everything else would be easily configurable by user. I really like it. A syntax would have to be decided and the scope of such configuration (only GPIO, things like RCC and GPIO, RCC + GPIO + interfaces, ...).

Again - the question is whether it's worth the trouble of implementing it. Linux is a general purpose OS, so people quite often take a stock board (like BBB) and just play with whatever it provides. It's important to note, that you rarely recompile Linux, but you need to alter the GPIO setting. On the other hand - distortos is not a general purpose OS, the configs for stock boards are - at least according to my current idea - a blend of a demo, configuration to run the test application and a base for real configuration for target project. You usually recompile whole project so there's no issue to change any configuration you like.

The thing is that in a final project - which you don't plan to be an example published in the repository - you can just configure anything you like at the beginning of main(), at the beginning of your threads or in your own board::lowLevelInitialization(). This reduces the value of a configuration system with completely new syntax which would require a good documentation to be usable by end users...

Generally the hardest things in a project like this are the decisions - actual programming is usually pretty easy compared to that (;

I've also thought that it would be a good idea to have a user lowLevelInitialization() function. This way you could use the board::lowLevelInitialization() from the repository, and then extend the configuration with whatever you need that is not fixed on the board. This way you wouldn't loose what the board provides (like LEDs and buttons) but still have the ability to extend it. Something in the spirit of "overlays" for BBB.

If I understand correctly you want to add some sets of pins for board, but how do you want to "abstract" for this configuration pins? There would be sets of pins groups like: board/serialPorts.hpp, board/spiMaster.hpp, board/leds.hpp, board/buttons.hpp? What if spi or rs485 communication device needs gpio pin for chip select. Do you plan to add board/gpioInputPins.hpp and board/gpioOutputPins.hpp or it would be included in serialPorts or spiMaster "objects"? I created my own spiPins with chip select pin, but I think it is a bad idea and it's better to create separated files with gpioInput and Output pins like for buttons and leds.

Yes, after second thought on that matter I also come to conclusion that this is not the best idea. Generally SpiMaster objects are NOT intended for direct use. My idea is that the user should only interact with SpiMaster via objects that contain/inherit-from SpiDevice and provide an appropriate interface (there can be many devices on single SPI bus). For now the only example of such class is SpiEeprom, but this should show my general idea. It's exactly how you did your own SpiLed7SegmentModule class in the 7SegmentSerialLEDModule project. So exposing SpiMaster objects for board (as a public header) is probably not such a good idea. The weakness of the idea is visible in the SerialPort vs. Rs485 dilemma - you cannot know what the user would like to do, so either you'd have to make that configurable (via Kconfig) or make an arbitrary decision. The configuration of pins is actually not such a big problem in that case, because you could easily document the requirements of such choices, for example enabling RS-485 interface for USART3 would require the user to provide an OutputPin object with name like rs485Port3Direction. But this again brings us the the basic question - is it worth all the trouble? Are people really going to use a stock board with NO fixed pin functions like that? Maybe it would just be better to make the configuration via source code as easy as possible, provide good documentation and a few well described examples in another repository? One example for "regular" serial port, one for RS-485, one for each implemented SPI device type and so on? It all comes down to the intended model of use. My vision (not necessarily "right" or "correct" - it's just how I do my projects) is that in a final project you don't really play with stock boards, you either have your own custom board or the stock board gets "customized" if you connect anything external via wires or expansion boards. In that case I just select "Custom board" in Kconfig and provide my own board::lowLevelConfiguration() or - if the project requires some level of configurations - I can provide that with Kconfig-board* files (as you did in your repository).

Actually, after your initial comment, I started thinking about removal of GPIO configuration from ChipInputPin and ChipOutputPin constructors to make that consistent with other objects provided by the project. You have to enable clocks in RCC anyway to use them, and you HAVE TO do that in board::lowLevelConfiguration() if the pin objects are global, so what difference would that make if you'd also have to configure the GPIO direction/mode/speed there?

I'm open to suggestions and ideas (; The whole idea of HAL in this project is not complete, so for now I've just implemented the first version. I'm still looking how this is done in other projects, but there are not that many examples available and those that are public always have some kind of issue...

CezaryGapinski commented 7 years ago

Hi! It's me again :). I thought for a long time about pins configurations issues and go back with the new idea. I really liked device trees but as you mentioned that would be too hard to develop something similar. But after a few hours of searching I found something easier. Syntax for xml files is complex but for json files looks more friendly and suitable for pin configurations requirements. So I created a small script in Python to generate board .cpp and .hpp files based on configuration written in json format. You can see this here: https://github.com/CezaryGapinski/DistortosPinConfigGenerator

Example with pin configuration file is in configuration/pin_config.json and generated directory tree in examples/NUCLEO-L073RZ As you can see except the leds and buttons I created also file for spi configurations, so here is my class for representing Spi Pins objects and here I added alternative pin configuration like for input and output pins. Of course is just suggestion/idea, but maybe it helps you to invent something smarter. I would like to ask you what do you think about that?

FreddieChopin commented 7 years ago

Hi Cezary!

Sorry for the delayed reply - recently it's hard to find some free time...

I like your idea very very much! The idea of having simple way to configure the board and generate the complete sources with everything that is needed is really tempting. When I first saw it (even before you posted the comment above (; ) I unleashed my mind and saw a nice application with a GUI that could be used to generate these configuration files - something in the spirit of CubeMX, but obviously tailored for distortos. But having something like that is probably pretty far away given the amount of work it would take to develop it...

But back to the subject. The idea is really good and I would like to follow it. However this creates a small crack in the configuration - some parts can be configured via Kconfig, some need code generation via a script like the one you wrote. You have probably noticed that one of the most complex pieces of Kconfig (and the accompanying preprocessor conditionals) is related to the clock configuration of the chip. So I started to wonder what else could be moved out of Kconfig into such generator. Probably quite a lot, but while things like LEDs, buttons and interfaces are rarely modified once done, I can imagine someone playing with the clock more often... Moreover the clock configuration code is currently part of the "chip", not "board", and a clear boundary of what the script will generate is pretty important, so I think that it should generate only the stuff for "board".

However if something simple - in the spirit of current make menuconfig - could be done directly in Python, we could think about moving the configuration system completely there. This would be much more intuitive to configure everything in one place instead of having board configuration completely separated from the general configuration which is currently done in Kconfig.

It would be pretty nice if the script could get the templates and/or the script fragments from different parts of the source tree - for example I guess it would be easier to maintain if the script fragment and/or template for GPIOv1 was located in the folder with GPIOv1 implementation. Otherwise the script would probably quickly become extremely long and hard to maintain. Such script fragments could also declare requirements of specific objects, which could be used for validation of the input configuration file.

I also think the script could generate the complete "board/your-selected-name" contents, including Kconfig-board*, Rules.mk, Tupfile.lua and whatever will be needed.

So much to think about (; Generally the direction is very good!

As for the additional classes you wrote - I guess this all could be done inside lowLevelInitialization() function for board - having these pins as objects just for the sake of having them initialized in the constructor is a waste of RAM and flash. With such generator in place we could think about moving the configuration of the LEDs and buttons there too, and making the constructors constexpr. For now there's no value in having these things as classes, as currently distortos does not support dynamic reconfiguration of GPIOs/hardware anyway. Such feature would be probably useful in 1% of all cases, greatly complicating the code. And these 1% of cases are highly specialized, for example when some lines are shared between several SPIs or maybe between two different interfaces - not very common use scenario.

BTW - maybe it would be better to use YAML instead of JSON - the syntax of YAML has a potential to be even simpler then JSON...

CezaryGapinski commented 7 years ago

Hi Freddie (or Kamil :))!

No problem, there is no rush at all, I saw you were involved in openOCD for quite some time. For me is better to make modest leap forward than jump to generating something too big like CubeMX. So in first step I just plan to generate files for board based on json and jinja, then yaml. I look at similarities and differences and at first glance that wouldn't be hard to put forward the solution with yaml. But that would be second step.

Let's go back to subject. If I correct understand the directory tree should looks like below?: distortos/scripts/DistPinCfgGen.py <- main script for generating board files used somewhere in makefile to generate cpp and hpp files

Specific sripts and files files for Pins used in main script: distortos/source/chip/peripherals/GPIOv1/boardScripts/GpioPinCfgGen.py <- specific script for gpio 1 distortos/source/chip/peripherals/GPIOv1/boardScripts/GpioRangeTemplates/ <- schema with allowed value ranges based on JSON-Schema and .jinja files with templates distortos/source/chip/peripherals/GPIOv2/boardScripts/GpioPinCfgGen.py <- specific script for gpio v2 distortos/source/chip/peripherals/GPIOv2/boardScripts/GpioRangeTemplates/ <- schema with allowed value ranges based on JSON-Schema and .jinja files with templates

File with pin config: [path specified as script line argument]/pin_config.json

Files to generate: [path specified as script line argument]/BOARD-Type-buttons.cpp [path specified as script line argument]/BOARD-Type-leds.cpp [path specified as script line argument]/BOARD-Type-lowLevelInitialization.cpp [path specified as script line argument]/Kconfig-boardChoices <- for board is necesessary to select chip and package [path specified as script line argument]/Kconfig-boardOptions <- just with config BOARD_INCLUDES and config BOARD [path specified as script line argument]/Rules.mk [path specified as script line argument]/Tupfile.lua [path specified as script line argument]/include/distortos/board/buttons.hpp [path specified as script line argument]/include/distortos/board/leds.hpp

If there is no objects to for SpiPins, ChipAlternativePin, these configurations should be generated direct in BOARD-Type-lowLevelInitialization.cpp in void lowLevelInitialization(). For example:

void lowLevelInitialization()
{
    RCC->IOPENR |=
    RCC_IOPENR_GPIOCEN |
    RCC_IOPENR_GPIOAEN |
            0;

    //spi1_clk
    configureAlternateFunctionPin( chip::Pin::pa5,
        false,
        chip::PinOutputSpeed::veryHigh,
        chip::PinPull::up,
        chip::PinAlternateFunction::af0
    );

    //spi1_miso
    configureAlternateFunctionPin( chip::Pin::pa11,
        false,
        chip::PinOutputSpeed::veryHigh,
        chip::PinPull::up,
        chip::PinAlternateFunction::af0
    );

    //spi1_mosi
    configureAlternateFunctionPin( chip::Pin::pa12,
        false,
        chip::PinOutputSpeed::veryHigh,
        chip::PinPull::up,
        chip::PinAlternateFunction::af0
    );
}

In this file should also contain configurations for analog pins configureAnalogPin(...)?

Because dynamically changes for pins is not common useful that's why I can get rid of SpiPins, ChipAlternativePin objects. But for example SpiDevice still need output pin for slaveSelectPin, thats some kind of object to represent gpio output pin is needed. My proposition is to add files: [some specified path]/BOARD-Type-generalOutputPins.cpp [some specified path]/include/distortos/board/generalOutputPins.hpp [some specified path]/BOARD-Type-generalInputPins.cpp [some specified path]/include/distortos/board/generalInputPins.hpp

based on leds and buttons files structure, which contain gpioPins to use for ex. in SpiDevice, RS485 device, etc.?

Second proposal: I would like to remove configs for BOARD_BUTTONS_ENABLE and BOARD_LEDS_ENABLE from Kconfig. If someone need buttons or leds then specified pins will be generated based on pin_config file.

Of course you have right, there is a lot of thinks to do and thanks for your answers and perhaps is better to create new version to see how it would be look like.

FreddieChopin commented 7 years ago

For me is better to make modest leap forward than jump to generating something too big like CubeMX.

Agreed - we have to start with something small and then improve.

Let's go back to subject. If I correct understand the directory tree should looks like below?: distortos/scripts/DistPinCfgGen.py <- main script for generating board files used somewhere in makefile to generate cpp and hpp files

If you ask me, I would prefer this script to be called generateBoard.py. The path is obviously fine.

Specific sripts and files files for Pins used in main script: distortos/source/chip/peripherals/GPIOv1/boardScripts/GpioPinCfgGen.py <- specific script for gpio 1 distortos/source/chip/peripherals/GPIOv1/boardScripts/GpioRangeTemplates/ <- schema with allowed value ranges based on JSON-Schema and .jinja files with templates distortos/source/chip/peripherals/GPIOv2/boardScripts/GpioPinCfgGen.py <- specific script for gpio v2 distortos/source/chip/peripherals/GPIOv2/boardScripts/GpioRangeTemplates/ <- schema with allowed value ranges based on JSON-Schema and .jinja files with templates

I would drop the boardScripts part, I guess there won't be so much files there to justify separate folder. And as I wrote to you in a comment in your repository, I think for now we can ignore validation. We just need to keep it in mind when writing the main script to make it easier to accommodate such validation in the future, but let's not overthink that now. Do we need the .py script here? Wouldn't a .jinja template be enough here?

File with pin config: [path specified as script line argument]/pin_config.json

Files for currently supported boards should probably be next to distortosConfiguration.mk files in the configurations/ folder. Maybe not at the lowest level, but right under board name - so for example not in distortos/configurations/32F429IDISCOVERY/test/ but in distortos/configurations/32F429IDISCOVERY/. I would prefer the file to be called sth like boardConfiguration.json or maybe boardSource.json - it would be used for more things than just pins.

Files to generate: ...

It would be great if the script would be able to generate all the files which are present there for now. This would be a good first step, so only buttons and LEDs would be enough initially, we'll add more features later. It would be great if the generated files would be similar to current files, but I'm open to some changes there.

If there is no objects to for SpiPins, ChipAlternativePin, these configurations should be generated direct in BOARD-Type-lowLevelInitialization.cpp in void lowLevelInitialization(). For example:

Yup - exactly what I thought about!

RCC->IOPENR |= RCC_IOPENR_GPIOCEN | RCC_IOPENR_GPIOAEN | 0;

If there will be no #ifdef and stuff like that you can drop the 0 at the end - it's there only because of the preprocessor.

In this file should also contain configurations for analog pins configureAnalogPin(...)?

Hard to tell now, as there's no support for ADC/DAC in the source. I think this should be ignored for now (;

Because dynamically changes for pins is not common useful that's why I can get rid of SpiPins, ChipAlternativePin objects. But for example SpiDevice still need output pin for slaveSelectPin, thats some kind of object to represent gpio output pin is needed. My proposition is to add files: [some specified path]/BOARD-Type-generalOutputPins.cpp [some specified path]/include/distortos/board/generalOutputPins.hpp [some specified path]/BOARD-Type-generalInputPins.cpp [some specified path]/include/distortos/board/generalInputPins.hpp

The general idea is that slave select pins:

Something like this is true also for RS-485 and probably anything more we'll come up with in the future.

Thus I think that such pins should be generated as local objects (in anonymous namespace) in .cpp files for particular device. But let's postpone that to next step - let's focus on buttons and LEDs for now, as a nice base functionality.

Second proposal: I would like to remove configs for BOARD_BUTTONS_ENABLE and BOARD_LEDS_ENABLE from Kconfig. If someone need buttons or leds then specified pins will be generated based on pin_config file.

It depends on how you see the script used. For now - as long as this script does not manage whole configuration of the project - I think running this script should not be part of the regular build step (make all or tup). Moreover - if this script is used to generate some Kconfig-* files, then it actually must be executed as separate step on demand. I would then propose to leave the BOARD_ENABLE_... options as they are now. So the script would either be executed directly or maybe like make board SOME=options OTHER=option.

In the future - if these .json files would contain whole configuration of the project, effectively replacing distortosConfiguration.mk files and Kconfig, then the script could be executed as part of build step, but I'm not entirely sure it would actually be needed. It all depends on what we'll come up with regarding the tool to generate/edit these .json files...

CezaryGapinski commented 7 years ago

Let's go back to subject. If I correct understand the directory tree should looks like below?: distortos/scripts/DistPinCfgGen.py <- main script for generating board files used somewhere in makefile to generate cpp and hpp files

If you ask me, I would prefer this script to be called generateBoard.py. The path is obviously fine.

You have right that name is more suitable if script would be used to generate more than pins.

I would drop the boardScripts part, I guess there won't be so much files there to justify separate folder. And as I wrote to you in a comment in your repository, I think for now we can ignore validation. We just need to keep it in mind when writing the main script to make it easier to accommodate such validation in the future, but let's not overthink that now. Do we need the .py script here? Wouldn't a .jinja template be enough here?

Take a look on my repo with scirpt. I moved templates to peripherals/GPIOv2/jinjaTemplates/ and I would like to put templates in simillar path in distortos sources. But I still hesitate if it is good path because for ex. peripherals/GPIOv2 are directed to chip, but LEDs Buttons lowLevelInit are specific to boards.

It would be great if the script would be able to generate all the files which are present there for now. This would be a good first step, so only buttons and LEDs would be enough initially, we'll add more features later. It would be great if the generated files would be similar to current files, but I'm open to some changes there.

KConfig and rules are also generated, but... In boardConfiguration.json file I have to add new options for rcc, vdd :) It's good or bad? (look at config for 32F429IDISCOVERY

If there will be no #ifdef and stuff like that you can drop the 0 at the end - it's there only because of the preprocessor.

For now it is easier for me to generate file with 0, but ok, I will think about it.

But let's postpone that to next step - let's focus on buttons and LEDs for now, as a nice base functionality.

I totally agree, it is better to start and finish one step and then focus on another features. As you can see I also removed spiPins and schema for checking correctness of values in configuration file.

For me the hardest think is to choose correct directories to put templates (or maybe scripts) in distortos tree. What do you think, in this first step peripherals/GPIOx2/jinjaTemplates/ are correct paths for jinja templates?

FreddieChopin commented 7 years ago

Take a look on my repo with scirpt. I moved templates to peripherals/GPIOv2/jinjaTemplates/ and I would like to put templates in simillar path in distortos sources. But I still hesitate if it is good path because for ex. peripherals/GPIOv2 are directed to chip, but LEDs Buttons lowLevelInit are specific to boards.

Let me start by introducing the general concept I have about directories. I assume that end-user may wish to significantly reduce the number of files in the project folder. For example if one is writing a project for STM32F1, he/she won't need the files for any other STM32, any other chip and any other architecture than ARMv7-M. That's why somewhere between v0.1.0 and v0.2.0 I added a feature which allows some of the folders to be entirely deleted. BTW that's the main reason why some Kconfig files are auto-generated. The idea here is that it should be relatively easy to just remove parts of distortos which are not needed for a particular project. This means that I try to keep everything as isolated as possible - all files related to STM32 should be in chip/STM32/ (some are in external/ but this is another story).

That's why I would really like the chip-specific or peripheral-specific parts to be kept in the folder of that chip/peripheral.

Now I think I have a solution for this problem, which would actually greatly reduce overall complexity. Let's split the template for LEDs, buttons, ... into two parts. One part is completely generic and independent of hardware. This would be the "outer" template, as it would contain general file structure, header, banners, some names etc. Second part (or parts, I assume there might be more than 1 "fragment" needed) would have only hardware specific stuff. It would be "inner" part, as it would contain the contents of arrays, actual initializers and things like that. I think that such separation can be done for any other file for boards, and if not currently, then we can surely tweak current files to make them fit that idea (; In that case "generic" script/templates can be placed in scripts/some/other/path/ and specific parts near particular chip/peripheral.

For example now I'm comparing ...-leds.cpp file for NUCLEO-F103RB and STM32F4DISCOVERY. Almost whole file is completely generic, only the contents of the array should be platform specific (in this example even that could be generic, but let's assume that the initializers might actually be different.

If I compare leds.hpp for these boards, the situation is really similar - I think that in case of this file 100% is generic and platform independent.

In that case note that actually it doesn't matter if the "inner" part of template (the one with actual chip::ChipOutputPin initializer) is used for LED, chip select, RS-485 direction pin or anything else - the initializer would be identical anyway, only the "outer" structure of the file is different.

Another idea - maybe instead of things like "gpio_driver_version":"v2", we could just have a path to apropriate folder in the repository (in this case something like STM32/peripherals/GPIOv2/ or maybe even peripherals/GPIOv2/, as STM32/ may be kept in another configuration variable? Maybe this way the generators for almost anything could be made more generic, as the path would allow you to load appropriate script fragment or template and just parse it?

KConfig and rules are also generated, but... In boardConfiguration.json file I have to add new options for rcc, vdd :) It's good or bad? (look at config for 32F429IDISCOVERY

I guess that we have no other option now. You have to add them somewhere anyway, so we may as well keep all of that in single .json file.

For now it is easier for me to generate file with 0, but ok, I will think about it.>

BTW - another simplification for the lowLevelInitialization() part which enables RCCs. You may as well generate something like this:

    RCC->AHB1ENR |=
#ifdef CONFIG_BOARD_BUTTONS_ENABLE
            RCC_AHB1ENR_GPIOAEN |
#endif  // def CONFIG_BOARD_LEDS_ENABLE
#ifdef CONFIG_BOARD_BUTTONS_ENABLE
            RCC_AHB1ENR_GPIOAEN |
#endif  // def CONFIG_BOARD_LEDS_ENABLE
#ifdef CONFIG_BOARD_BUTTONS_ENABLE
            RCC_AHB1ENR_GPIOAEN |
#endif  // def CONFIG_BOARD_LEDS_ENABLE
#ifdef CONFIG_BOARD_BUTTONS_ENABLE
            RCC_AHB1ENR_GPIOCEN |
#endif  // def CONFIG_BOARD_LEDS_ENABLE
#ifdef CONFIG_BOARD_LEDS_ENABLE
            RCC_AHB1ENR_GPIOAEN |
#endif  // def CONFIG_BOARD_LEDS_ENABLE
#ifdef CONFIG_BOARD_LEDS_ENABLE
            RCC_AHB1ENR_GPIOBEN |
#endif  // def CONFIG_BOARD_LEDS_ENABLE
#ifdef CONFIG_BOARD_LEDS_ENABLE
            RCC_AHB1ENR_GPIODEN |
#endif  // def CONFIG_BOARD_LEDS_ENABLE
#ifdef CONFIG_BOARD_LEDS_ENABLE
            RCC_AHB1ENR_GPIODEN |
#endif  // def CONFIG_BOARD_LEDS_ENABLE
            0;

or sth like that:

    RCC->AHB1ENR |=
#ifdef CONFIG_BOARD_BUTTONS_ENABLE
            RCC_AHB1ENR_GPIOAEN |
            RCC_AHB1ENR_GPIOAEN |
            RCC_AHB1ENR_GPIOAEN |
            RCC_AHB1ENR_GPIOCEN |
#endif  // def CONFIG_BOARD_BUTTONS_ENABLE
#ifdef CONFIG_BOARD_LEDS_ENABLE
            RCC_AHB1ENR_GPIOAEN |
            RCC_AHB1ENR_GPIOBEN |
            RCC_AHB1ENR_GPIODEN |
            RCC_AHB1ENR_GPIODEN |
#endif  // def CONFIG_BOARD_LEDS_ENABLE
            0;

I mean you can repeat the values as many times as you want, for every single pin that is configured. The compiler will optimize that anyway, so no problem here. In your case it may simplify things, as you won't have to merge pins and then divide them into "pins for LEDs", "pins for buttons", "pins for whatever that can be enabled/disabled" and so on.

BTW - I think it would be possible to add one (or as many as needed) level of indirection here and have defines/constexprs/functions/sth-else like RCC_GPIOA_ENABLE, which would then - depending on particular chip be transformed into RCC_AHBENR_GPIOAEN, RCC_APB2ENR_IOPAEN or whatever. I assume that assigning the GPIO to proper bus (AHB, AHB1, AHB2, APB, APB1, APB2, ...) and proper "name" (IOPxEN / GPIOxEN) may be non-trivial.

For me the hardest think is to choose correct directories to put templates (or maybe scripts) in distortos tree. What do you think, in this first step peripherals/GPIOx2/jinjaTemplates/ are correct paths for jinja templates?

I hope that with the explanation of the original idea behind the folder structure and removal of unneeded directories you have a better understanding of the concept that I'm after. And maybe the idea of further dividing of the templates into "inner" and "outer" part will also appeal to you (;

FreddieChopin commented 7 years ago

As for all the RCC_AHB1ENR_GPIOAEN, RCC_AHB1ENR_GPIOCEN, ... - there is yet another option, which might be the best one. Add options like "Enable GPIOA", "Enable GPIOB", ... to Kconfig, just as with "Enable USARTx" or "Enable SPIx". This would allow for following things:

CezaryGapinski commented 7 years ago

Hi!

As for all the RCC_AHB1ENR_GPIOAEN, RCC_AHB1ENR_GPIOCEN, ... - there is yet another option, which might be the best one. Add options like "Enable GPIOA", "Enable GPIOB", ... to Kconfig, just as with "Enable USARTx" or "Enable SPIx".

Sounds good, chip::lowLevelInitialization() depend on specific chip. GPIO clock enable registers (RCC_AHB1ENR_GPIO for stm32f4, RCC_APB2ENR_IOP for stm32f1, RCC_AHBENR_GPIO for stm32f0) also are more related to chip than board.

if particular port is not enabled, values of chip::Pin would not be available, so you wouldn't be able to use such GPIO pin in a "normal" way - good for avoiding strange errors.

If I correct understand for led or buttons (or in internal part of template like you mentioned in previous post):

Now I think I have a solution for this problem, which would actually greatly reduce overall complexity. Let's split the template for LEDs, buttons, ... into two parts. One part is completely generic and independent of hardware. This would be the "outer" template, as it would contain general file structure, header, banners, some names etc. Second part (or parts, I assume there might be more than 1 "fragment" needed) would have only hardware specific stuff. It would be "inner" part, as it would contain the contents of arrays, actual initializers and things like that. I think that such separation can be done for any other file for boards, and if not currently, then we can surely tweak current files to make them fit that idea (; In that case "generic" script/templates can be placed in scripts/some/other/path/ and specific parts near particular chip/peripheral.

Then source file generated for leds or buttons should be "surrounded with #ifdef's": ex: 32F429IDISCOVERY-buttons.cpp

const chip::ChipInputPin buttons[totalButtons]
{

#ifdef CONFIG_CHIP_STM32F4_RCC_AHB1ENR_GPIOA_ENABLE
    /// configuration for button B1
    chip::ChipInputPin{ chip::Pin::pa0, chip::PinPull::none, false }, 
#endif

};

where CONFIG_CHIP_STM32F4_RCC_AHB1ENR_GPIOA_ENABLE will be generated thanks to new KConfig-chipOptions parameters?

CezaryGapinski commented 7 years ago

or easier CONFIG_CHIP_GPIOA_ENABLE?

FreddieChopin commented 7 years ago

Yes, it would be exactly the same as is done with low-level UART or SPI drivers - they are enabled only when particular option is enabled in Kconfig. Something like this:

https://github.com/DISTORTEC/distortos/blob/master/source/chip/STM32/peripherals/USARTv1/STM32-USARTv1-uarts.cpp#L26

I think the defines would be similar, so CONFIG_CHIP_STM32_GPIOV1_GPIOA_ENABLE and so on.

A slight problem would be in the definition of things like totalButtons, because now you could selectively disable some LEDs/buttons/whatevers... If there would be LEDs on ports A, B and C, then enabling just A and C would result in a smaller number of LEDs than in the case of enabling them all. I have to think about that some more, but generally such "inversion of logic" seems reasonable and consistent with what was done for UART/SPI and with what I've seen in other projects.

Another problem is that it would make the separation of templates into "inner" and "outer" parts a bit more complicated - I think that either the .json file would have to have info about option required for particular object (for example green LED is enabled only if GPIOA is enabled too) or you'd have to generate that info from the pin's port name.

Hmm... This solution is not perfect (at least now), but I think that it's the correct direction...

CezaryGapinski commented 7 years ago

Hi!

A slight problem would be in the definition of things like totalButtons, because now you could selectively disable some LEDs/buttons/whatevers... If there would be LEDs on ports A, B and C, then enabling just A and C would result in a smaller number of LEDs than in the case of enabling them all. I have to think about that some more, but generally such "inversion of logic" seems reasonable and consistent with what was done for UART/SPI and with what I've seen in other projects.

How about something similar to this (example for 32F429IDISCOVERY):

32F429IDISCOVERY-leds.cpp:

#include "distortos/board/leds.hpp"

#ifdef CONFIG_BOARD_LEDS_ENABLE

#include "distortos/chip/ChipOutputPin.hpp"

namespace distortos
{

namespace board
{

/*---------------------------------------------------------------------------------------------------------------------+
| global objects
+---------------------------------------------------------------------------------------------------------------------*/

chip::ChipOutputPin leds[totalLeds]
{  
#ifdef CONFIG_CHIP_STM32_GPIOV2_GPIOA_ENABLE
    /// configuration for led LD4
    chip::ChipOutputPin{ chip::Pin::pa12,
    false,
    chip::PinOutputSpeed::low,
    chip::PinPull::none,
    false,
    false },
#endif
#ifdef CONFIG_CHIP_STM32_GPIOV2_GPIOG_ENABLE
    /// configuration for led LD3
    chip::ChipOutputPin{ chip::Pin::pg13,
    false,
    chip::PinOutputSpeed::low,
    chip::PinPull::none,
    false,
    false },
#endif
};

}   // namespace board

}   // namespace distortos

#endif  // def CONFIG_BOARD_LEDS_ENABLE

leds.hpp

#ifndef SOURCE_BOARD_STM32_STM32F4_32F429IDISCOVERY_INCLUDE_DISTORTOS_BOARD_LEDS_HPP_
#define SOURCE_BOARD_STM32_STM32F4_32F429IDISCOVERY_INCLUDE_DISTORTOS_BOARD_LEDS_HPP_

#include "distortos/chip/STM32-GPIOv2.hpp"

#include <array>

namespace distortos
{

#ifdef CONFIG_BOARD_LEDS_ENABLE

namespace chip
{

class ChipOutputPin;

}   // namespace chip

#endif  // def CONFIG_BOARD_LEDS_ENABLE

namespace board
{

enum Leds {
#ifdef CONFIG_CHIP_STM32_GPIOV2_GPIOA_ENABLE
    ld3LedIndex, /// index of LD3 LED (green)
#endif
#ifdef CONFIG_CHIP_STM32_GPIOV2_GPIOG_ENABLE
    ld4LedIndex, /// index of LD4 LED (red)
#endif
    ldLastInSeq, // must be last value
};

/// total number of LEDs on the board
constexpr size_t totalLeds {ldLastInSeq};

/*---------------------------------------------------------------------------------------------------------------------+
| alternative (color-based) LED indexes
+---------------------------------------------------------------------------------------------------------------------*/

#ifdef CONFIG_CHIP_STM32_GPIOV2_GPIOA_ENABLE
/// alternative index of LD3 LED (green) 
constexpr size_t greenLedIndex {ld3LedIndex};
#endif
#ifdef CONFIG_CHIP_STM32_GPIOV2_GPIOG_ENABLE
/// alternative index of LD4 LED (red) 
constexpr size_t redLedIndex {ld4LedIndex};
#endif

#ifdef CONFIG_BOARD_LEDS_ENABLE

/*---------------------------------------------------------------------------------------------------------------------+
| indexed access to LED objects
+---------------------------------------------------------------------------------------------------------------------*/

/// array with all LED objects
extern chip::ChipOutputPin leds[totalLeds];

#endif  // def CONFIG_BOARD_LEDS_ENABLE

}   // namespace board

}   // namespace distortos

#endif  // SOURCE_BOARD_STM32_STM32F4_32F429IDISCOVERY_INCLUDE_DISTORTOS_BOARD_LEDS_HPP_

So my general idea is to use enum where last value will be a number of leds to access. That number will be dependant to enabled/disabled clock signals for gpio.

FreddieChopin commented 7 years ago

Seems good, but in that case I would just drop separate totalLeds and have that in the enum - in place of ldLastInSeq. The comma after that final element should be removed, to make the "this must by last" requirement as explicit as possible.

BTW - the values in the array are in wrong order (;

CezaryGapinski commented 7 years ago

I think it is better place to discuss something more about script.

I already read your posts, and thats my plan:

  1. Update my forked project from remote distortec/master head.
  2. Create new branch named for ex. board-script-generator (it is saver for me instead of rebasing and push forced changes to existing one - I'm afraid I can miss something out and ruin my work :))
  3. Adjust all files in distortos to make them friendly for script (change all files to expected state):
    • change manually all headers, source files, Kconfig, etc. to the state which should be generated by script (then I can always see the differences when script will be doing some inappropriate thinks)
  4. Looking forward for comments and sounds remarks and make the fixes (or if you have some already when you looked on existing board_script branch you can let me know now - for example it is good with actual enum in leds.hpp or buttons.hpp or it should be class enum with the type uint32_t).
  5. The most important - add script, templates and modify makefile (or if you can tell me now if is good approach to create paths where jinja templates can be found in makefile based on DIRECTORIES variable:
# generated directories with paths where jinja templates should be searched 
JINJA_DIRECTORIES      :=  $(subst output/,,$(DIRECTORIES)) 
JINJA_DIRECTORIES      += scripts/templates 

python $(DISTORTOS_PATH)scripts/generateBoard.py -c $(CONFIG_BOARD) -o $(OUTPUT_BOARD) -s $(JINJA_DIRECTORIES) 
  1. Regenerate all board files.
  2. Make a pull request to your repository.
FreddieChopin commented 7 years ago

(it is saver for me instead of rebasing and push forced changes to existing one - I'm afraid I can miss something out and ruin my work :))

Actually even if you do force push, the history is still there in your repository's "reflog". In git nothing is ever lost (; Even if you have a stash and delete it, it is not lost (;

(or if you have some already when you looked on existing board_script branch you can let me know now - for example it is good with actual enum in leds.hpp or buttons.hpp or it should be class enum with the type uint32_t).

Definitely leave that as normal "enum" - you wouldn't be able to use "enum class" as array indices (without casts).

From looking at the script, my only remark now is that the naming style (you use under_scores to separate words) is not consistent with what I used in the whole project (camelCase).

Oh, and this thing too - https://github.com/DISTORTEC/distortos/issues/25#issuecomment-279179798

or if you can tell me now if is good approach to create paths where jinja templates can be found in makefile based on DIRECTORIES variable

Hmm... Wouldn't it be easier to just search for the files directly from python? If not, maybe just use shell with find (but if you do, please note that there are some problems with that too, so let me know and I'll tell you what's the issue). The problem with the $(DIRECTORIES) variable (at least I suspect that this may be a problem) is that it is evaluated only if you have "configured" your project (that is - you executed make configure ...). Otherwise - if the project is NOT configured - running make will produce an error because selectedConfiguration.mk file cannot be found. Now this may be a problem, because it doesn't make much sense to configure the project for board generation. Even if you did that, then please note, that $(DIRECTORIES) does NOT contain all folders with sources - it only contains the directories that will be compiled (so for example only STM32F0, GPIOv2 and SPIv2, but not STM32F1, STM32F4, GPIOv1, SPIv1 and so on.

I think it is better place to discuss something more about script.

BTW - there's a mailing list and I was thinking about starting a forum, but I'm not sure anyone is going to participate right now. Any thoughts on that?

FreddieChopin commented 7 years ago

One more thing - as I wrote in the other thread - please try to make smaller and more frequent pull requests. I'm talking especially about the stuff from your items nr 3 and 4, as these changes can (and should) be merged even before the script. It will be easier for all of us.

CezaryGapinski commented 7 years ago

Hi!

I've started to rebase and create more consistent commits. I've started from changes with buttons.hpp in NUCLEO-F091RC, but the problem exist also for the other boards.

If you look at Kconfig-board:

config BOARD_BUTTONS_ENABLE
    bool "Enable buttons"
    depends on BOARD_TOTAL_BUTTONS != 0
    default y
    help
        Enables board buttons.

The BOARD_BUTTONS_ENABLE depend on BOARD_TOTAL_BUTTONS but this variable will be moved to the last enum value in buttons.hpp, so is invisible in Kconfig and the board's buttons enable option doesn't show up in menuconfig.

Is BOARD_LEDS_ENABLE still needed? For example when someone creates board config file without buttons then buttons.hpp and NUCLEO-F091RC-buttons.cpp could be empty?

FreddieChopin commented 7 years ago

In that case I'd say that BOARD_TOTAL_BUTTONS and BOARD_TOTAL_LEDS variables in Kconfig-boardOptions should be changed to BOARD_HAS_BUTTONS & BOARD_HAS_LEDS, which would be bool.

As the arrays cannot be empty (gcc allows that, but this is only a non-standard extension), I think we would also need the condition to be a little more complex. In the generaged Kconfig files you'd have to output sth like:

config BOARD_HAS_LEDS
    bool
    default y if CHIP_STM32_GPIOV1_GPIOA_ENABLE || CHIP_STM32_GPIOV1_GPIOC_ENABLE

That is - LEDs would depend on at least one GPIO connected to LEDs being enabled and the same for buttons. In that case the array would always have at least one element. Your generator knows which GPIOs (and what version) is used, so it may generate that info without problems.

Then the "main" Kconfig-boardOptions would need just a trivial change to:

config BOARD_BUTTONS_ENABLE
    bool "Enable buttons"
    depends on BOARD_HAS_BUTTONS
    default y
    help
        Enables board buttons.
CezaryGapinski commented 7 years ago

You have right, it is better to focus on only one specific issue and it helps me to not overlook above problem. I created some changes on my board_script branch, but I also created new branch only for moving buttons and leds indexes to enum

config BOARD_HAS_LEDS
    bool
    default y if CHIP_STM32_GPIOV1_GPIOA_ENABLE || CHIP_STM32_GPIOV1_GPIOC_ENABLE

But I know when someone define "leds" group in json config file and this condition can looks much easier (please see my changes on created branch). I'm able to add select BOARD_HAS_BUTTONS to Kconfig-BoardChoices rather than create logic with CHIP..._GPIOA_ENABLE || CHIP..._GPIOC_ENABLE.

FreddieChopin commented 7 years ago

But I know when someone define "leds" group in json config file and this condition can looks much easier (please see my changes on created branch). I'm able to add select BOARD_HAS_BUTTONS to Kconfig-BoardChoices rather than create logic with CHIP..._GPIOA_ENABLE || CHIP..._GPIOC_ENABLE.

This is not enough. A board can have LEDs, but the user doesn't have to enable any of the GPIO ports, so the array will end up being empty. You can use the select ... construct instead of what I proposed (they are functionally equivalent for bools), but it also needs to be conditional on the ports, so something like this:

select BOARD_HAS_BUTTONS if CHIP_STM32_GPIOV1_GPIOA_ENABLE
select BOARD_HAS_LEDs if CHIP_STM32_GPIOV1_GPIOA_ENABLE || CHIP_STM32_GPIOV1_GPIOC_ENABLE

As the generator will initially be based on JSON files only, which are not very human-friendly, and because the Kconfig based system should still work (until we replace it with something better), I'd prefer to have the option to enable/disable LEDs in Kconfig. For custom boards this is not very useful, as in fact any configuration is very rarely useful for custom boards. But for typical "development" boards (Nucleo, Discovery, ...) such option makes sense, as the user may prefer to drive all the LEDs with PWM or maybe handle the button with external interrupt, so in that case - instead of regenerating the board - it would be enough to just disable these features in Kconfig.

I think that these are the issues we'll be facing constantly because the script effectively creates another configuration stage, almost completely separated from the one based on Kconfig... Until we come up with an unified solution, I would like to keep Kconfig as usable as possible.

CezaryGapinski commented 7 years ago

Ok, I've made some changes. Do you think it is ok now? Shall I now finish next changes on this branch or move it to a former board_config branch?

Perhaps JSON is not that convenient like YAML, but I just want to finish current version with JSON. There is no such big problem to migrate to YAML in next version of script (I think so).

I think that these are the issues we'll be facing constantly because the script effectively creates another configuration stage, almost completely separated from the one based on Kconfig... Until we come up with an unified solution, I would like to keep Kconfig as usable as possible.

Agree, it is only way to develop board config file and keep up with actual Kconfig menus.

FreddieChopin commented 7 years ago

Ok, I've made some changes. Do you think it is ok now? Shall I now finish next changes on this branch or move it to a former board_config branch?

Yes, the general direction is correct. Would you be able to implement the same change for both buttons and LEDs, in all boards, or should I do it? When this change is complete (buttons & LEDs in all boards), we can merge it to the main repository.

Perhaps JSON is not that convenient like YAML, but I just want to finish current version with JSON. There is no such big problem to migrate to YAML in next version of script (I think so).

Sure, one step at a time (;

CezaryGapinski commented 7 years ago

Yes, the general direction is correct. Would you be able to implement the same change for both buttons and LEDs, in all boards, or should I do it? When this change is complete (buttons & LEDs in all boards), we can merge it to the main repository.

Ok, I can finish it for all boards.

CezaryGapinski commented 7 years ago

The "alternative" indexes for LEDs also need the same conditions as original indexes, otherwise you'd get a compilation failure when the port is disabled. The "alternative" indexes were moved "inside" the CONFIG_BOARD_LEDS_ENABLE condition. Without the removed table with pin identifiers, these are not needed when LEDs are disabled.

Ok, thanks for your fixes on last pull request, now the distortosExamples have to be updated too. BTW what is a purpose of alternative indexes for leds? It is used somewhere? It's not easier just name enum values in more appropriate way? For example ldOrangeLedIndex instead of ld3LedIndex?

FreddieChopin commented 7 years ago

now the distortosExamples have to be updated too.

I've implemented some more changes to make that happen and will get back to examples this evening (I hope so (; ).

BTW what is a purpose of alternative indexes for leds? It is used somewhere? It's not easier just name enum values in more appropriate way? For example ldOrangeLedIndex instead of ld3LedIndex?

Well, they are not used anywhere, but I think they are useful. But I don't think that changing the main indexes is the way to go, as depending on the situaion you may want one of the other or both. That's why now "main" indexes are based on the most general description - schematics - while the alternative indexes are just a convenience addition with colours.

This is one of the decisions that I had to make - whether I made a good one or not is another matter (; I'm certainly open to discussion.

CezaryGapinski commented 7 years ago

Hi!

Yesterday I pushed some new commits with board files generator here

Now the script is able to search for the template files in all subdirectories, dependently where is called. Optionally -s with paths argument can be used to search for jinja templates in the others directories.

I still hesitate how the header of file should looks. Now contain mention that is generated (it can be changed or removed), but another issue is a begins year when the particular file was started and actual year mentioned in header. Is it necessary to achieve compatibility with previous versions of files? Year can be different for each file, so it is hard to maintain this in script.

I've fixed coding style for script and template files (for example tabs and variables names with camelCase where it was possible).

As you can see there is also difference with #define for DISTORTOS_BOARD_TOTAL_LEDS and DISTORTOS_BOARD_TOTAL_BUTTONS but this style should be compatible with your suggestions in one of our previous post:

/// total number of LEDs on the board
#define DISTORTOS_BOARD_TOTAL_LEDS      ( \
        DISTORTOS_BOARD_LD3_LED_ENABLE + \
        DISTORTOS_BOARD_LD4_LED_ENABLE + \
        DISTORTOS_BOARD_LD5_LED_ENABLE + \
        DISTORTOS_BOARD_LD6_LED_ENABLE + \
        0)

Can you give me now some advices, telling me how now it is looks like for you? It's good or is totally screwed up ;)?

FreddieChopin commented 7 years ago

Yesterday I pushed some new commits with board files generator here

Yeah, I know - I noticed them some minutes after you uploaded that <:

I still hesitate how the header of file should looks. Now contain mention that is generated (it can be changed or removed), but another issue is a begins year when the particular file was started and actual year mentioned in header. Is it necessary to achieve compatibility with previous versions of files? Year can be different for each file, so it is hard to maintain this in script.

I think we could follow what is currently done for other generated files - linker script and some Kconfig files. For example linker script has following header:

/**
 * \file
 * \brief Linker script for STM32F429ZI chip:
 * - 2097152 bytes of rom at 0x8000000;
 * - 196608 bytes of ram at 0x20000000;
 * - 4096 bytes of bkpsram at 0x40024000;
 * - 65536 bytes of ccm at 0x10000000;
 *
 * \author Copyright (C) 2014-2016 Kamil Szczygiel http://www.distortec.com http://www.freddiechopin.info
 *
 * \par License
 * This Source Code Form is subject to the terms of the Mozilla Public License, v. 2.0. If a copy of the MPL was not
 * distributed with this file, You can obtain one at http://mozilla.org/MPL/2.0/.
 *
 * \warning
 * Automatically generated file - do not edit!
 *
 * \date 2017-03-09 22:18:15
 */

Generated Kconfig files:

#
# file: Kconfig-chipChoices
#
# Automatically generated file - do not edit!
#
# date: 2017-03-09 22:19:52
#

So I propose to do it like this:

Does that sound OK to you?

As you can see there is also difference with #define for DISTORTOS_BOARD_TOTAL_LEDS and DISTORTOS_BOARD_TOTAL_BUTTONS but this style should be compatible with your suggestions in one of our previous post: ...

It's perfect (;

Can you give me now some advices, telling me how now it is looks like for you? It's good or is totally screwed up ;)?

It looks very well and I hope to merge it soon (;

A couple of issues (; I hope I won't discourage you now (; They are mostly minor issues, so (I hope) not a big deal...

  1. You should add board target to SIMPLE_TARGETS in Makefile - otherwise your script cannot be used when distortos is not configured. Try to do make distclean + make board ... - it will fail exactly because of that.

  2. I think that the help text is misleading - square brackets suggest that the arguments are optional, while they are not.

  3. I also think that you should not use CONFIG_BOARD as the name of one argument - this variable is actually provided by distortosConfiguration.mk, so if you omit that argument (and when distortos is configured), you get:

$ make board OUTPUT_BOARD=xxx
python scripts/generateBoard.py -c "32F429IDISCOVERY" -o xxx
Traceback (most recent call last):
  File "scripts/generateBoard.py", line 222, in <module>
    if __name__ == '__main__': main()
  File "scripts/generateBoard.py", line 174, in main
    with open(inputParameters.configFile) as configDataFile:
FileNotFoundError: [Errno 2] No such file or directory: '32F429IDISCOVERY'
make: *** [Makefile:283: board] Error 1

Probably not what you want (; Maybe CONFIG_FILE? Then maybe OUTPUT_PATH instead of OUTPUT_BOARD?

  1. Am I right to assume that your script works with Python 2 only? On my Arch Linux (one of these strange bleeding-edge systems, where python actually refers to Python 3) I get:
$ make board OUTPUT_BOARD=xxx CONFIG_BOARD=configurations/32F429IDISCOVERY/boardConfiguration.json 
python scripts/generateBoard.py -c configurations/32F429IDISCOVERY/boardConfiguration.json -o xxx
Traceback (most recent call last):
  File "scripts/generateBoard.py", line 222, in <module>
    if __name__ == '__main__': main()
  File "scripts/generateBoard.py", line 203, in main
    data["gpioDriverVersion"])
  File "scripts/generateBoard.py", line 81, in getTemplateFileFromTypeAndVersion
    for fileName, parameters in outputTemplates.iteritems():
AttributeError: 'dict' object has no attribute 'iteritems'
make: *** [Makefile:283: board] Error 1

If I force use of Python 2 in the Makefile, it seems to work correctly. How hard would it be to make the script compatible with Python 2 and 3 at the same time? Not necessarily now, but in the future? For now if you know of no other way (would using python2 be generic and work on "all" platforms?), I suggest the make board to use $(PYTHON) instead of python, so that I can override that in the invocation. And maybe print some informative error ("This script currently requires Python 2, try make board PYTHON=pytnon2 ..." or sth like that) so that it would be obvious what is going on.

  1. You may have noticed, that I've given the path as just "xxx". This results in files being placed in "xxx32F429IDISCOVERY" folder. I think that your script should always add the trailing slash to the argument (or convert it to separate folder in any other way).

  2. I see that the script completely removes the path that is generated. After second thought I think it should be changed. At least because this would allow the .json files to be stored in a more natural location - source/board/... (along with the generated files) instead of configurations/.... For now it would be OK to have the script only overwrite the files it generates. Sorry if I suggested something opposite earlier... Using such location would also allow the second script argument to be optional - if no output path is given, the board is generated in the same folder as the one with .json file.

  3. Maybe scripts/generateBoard-templates/... (or ...generateBoardTemplates...) would be better than scripts/templates/...? Or maybe use ...jinjaTemplates... for all the folders with templates? I also think you can drop the BOARD_NAME- part from name of some templates, unless you need it for something.

  4. Header of the script has old name of the file. Maybe we could use a shorter version of the description? For example "Python script used to generate board configuration files" - the rest is pretty obvious, so no need to restate it. But this is your file, so you decide (; In any case, please use "distortos" as the name, not "Distortos".

CezaryGapinski commented 7 years ago
Yesterday I pushed some new commits with board files generator here

Yeah, I know - I noticed them some minutes after you uploaded that <:

Nice to know, I'm also up-to-date with your branches.

So I propose to do it like this:

  • Add yourself as the second (or first) author by duplicating the current line, with doxygen tags and copyright and so on.
  • For the copyright year I would propose to use date range - since the start of project (2014) to "current year". There's no need to keep it different for each file/board/config/...
  • Add info that the file is automatically generated in the similar format and position as in above examples). Maybe add info that it is generated by the script (I would omit the full path, just leave the name).
  • Add date of generation (full date and time), just as in the above examples.

Does that sound OK to you?

That's sounds alright to me.

As you can see there is also difference with #define for DISTORTOS_BOARD_TOTAL_LEDS and DISTORTOS_BOARD_TOTAL_BUTTONS but this style should be compatible with your suggestions in one of our previous post: ...

It's perfect (;

Please check twice, perhaps is somewhere a mess with "tabs" and "spaces" :>

A couple of issues (; I hope I won't discourage you now (; They are mostly minor issues, so (I hope) not a big deal...

No, as you know is easy to focus on one subject and miss other topics out :).

  1. You should add board target to SIMPLE_TARGETS in Makefile - otherwise your script cannot be used when distortos is not configured. Try to do make distclean + make board ... - it will fail exactly because of that.

  2. I think that the help text is misleading - square brackets suggest that the arguments are optional, while they are not.

  3. I also think that you should not use CONFIG_BOARD as the name of one argument - this variable is actually provided by distortosConfiguration.mk, so if you omit that argument (and when distortos is configured), you get:

$ make board OUTPUT_BOARD=xxx
python scripts/generateBoard.py -c "32F429IDISCOVERY" -o xxx
Traceback (most recent call last):
  File "scripts/generateBoard.py", line 222, in <module>
    if __name__ == '__main__': main()
  File "scripts/generateBoard.py", line 174, in main
    with open(inputParameters.configFile) as configDataFile:
FileNotFoundError: [Errno 2] No such file or directory: '32F429IDISCOVERY'
make: *** [Makefile:283: board] Error 1

Probably not what you want (; Maybe CONFIG_FILE? Then maybe OUTPUT_PATH instead of OUTPUT_BOARD?

Oh, ok, that is a bad think and need to be fix as soon as possible. But in situation where the file really do not exist for some reason it is good to receive message like above, or should I more focus on handling exceptions in my script and print more user friendly messages?

  1. Am I right to assume that your script works with Python 2 only? On my Arch Linux (one of these strange bleeding-edge systems, where python actually refers to Python 3) I get:
$ make board OUTPUT_BOARD=xxx CONFIG_BOARD=configurations/32F429IDISCOVERY/boardConfiguration.json 
python scripts/generateBoard.py -c configurations/32F429IDISCOVERY/boardConfiguration.json -o xxx
Traceback (most recent call last):
  File "scripts/generateBoard.py", line 222, in <module>
    if __name__ == '__main__': main()
  File "scripts/generateBoard.py", line 203, in main
    data["gpioDriverVersion"])
  File "scripts/generateBoard.py", line 81, in getTemplateFileFromTypeAndVersion
    for fileName, parameters in outputTemplates.iteritems():
AttributeError: 'dict' object has no attribute 'iteritems'
make: *** [Makefile:283: board] Error 1

If I force use of Python 2 in the Makefile, it seems to work correctly. How hard would it be to make the script compatible with Python 2 and 3 at the same time? Not necessarily now, but in the future? For now if you know of no other way (would using python2 be generic and work on "all" platforms?), I suggest the make board to use $(PYTHON) instead of python, so that I can override that in the invocation. And maybe print some informative error ("This script currently requires Python 2, try make board PYTHON=pytnon2 ..." or sth like that) so that it would be obvious what is going on.

Exactly, now script works with using Python 2. I don't know now what is incorrect and what should be changed to it would be compatible with version 2 and 3. Now sounds ok to me to print the informative error as you mentioned. But give me more time to think about that issue. Perhaps that is just a small issue.

  1. You may have noticed, that I've given the path as just "xxx". This results in files being placed in "xxx32F429IDISCOVERY" folder. I think that your script should always add the trailing slash to the argument (or convert it to separate folder in any other way).

Agreed and this is not a big deal :).

  1. I see that the script completely removes the path that is generated. After second thought I think it should be changed. At least because this would allow the .json files to be stored in a more natural location - source/board/... (along with the generated files) instead of configurations/.... For now it would be OK to have the script only overwrite the files it generates. Sorry if I suggested something opposite earlier... Using such location would also allow the second script argument to be optional - if no output path is given, the board is generated in the same folder as the one with .json file.

What in situation when in existing directory are for example header and source files for leds? Then in next step someone decide to remove it and just leaves the buttons. Then files for leds should be removed. It was easier for me to just remove whole directory and generate new files based on .json. Ok, I will fix it up, location of .json file in source/board looks good now for situation when no output path is given for the script. But in situation when we need to generate whole new folder for board with the second argument of script? Then .json file should be also copied to this folder?

  1. Maybe scripts/generateBoard-templates/... (or ...generateBoardTemplates...) would be better than scripts/templates/...? Or maybe use ...jinjaTemplates... for all the folders with templates? I also think you can drop the BOARD_NAME- part from name of some templates, unless you need it for something.

jinjaTemplates sounds great, that would be the same name for whole project. Now BOARD_NAME is used to replace this string to specified in .json board name in my script (sorry for that :)). Furthermore, BOARD_NAME-leds.cpp.jinja as a prefix suggest that the output file should be look in some similar way (with some prefix). File name leds.cpp.jinja suggests that output file should be leds.cpp without prefix.

  1. Header of the script has old name of the file. Maybe we could use a shorter version of the description? For example "Python script used to generate board configuration files" - the rest is pretty obvious, so no need to restate it. But this is your file, so you decide (; In any case, please use "distortos" as the name, not "Distortos".

Ok, simpler is better.

FreddieChopin commented 7 years ago

Please check twice, perhaps is somewhere a mess with "tabs" and "spaces" :>

We'll employ regexes for minor stuff like that later, as they are much better at spotting whitespace issues.

Oh, ok, that is a bad think and need to be fix as soon as possible. But in situation where the file really do not exist for some reason it is good to receive message like above, or should I more focus on handling exceptions in my script and print more user friendly messages?

What I meant in that case is that if I don't give the argument, $(CONFIG_BOARD) coming from distortosConnfiguration.mk is used and this messes things up, as it is not a path - just a name. Don't worry about error message - what is printed by Python here is good enough. I would just prefer to use something else than CONFIG_BOARD, for example CONFIG_FILE (and maybe OUTPUT_PATH as the second argument).

Exactly, now script works with using Python 2. I don't know now what is incorrect and what should be changed to it would be compatible with version 2 and 3. Now sounds ok to me to print the informative error as you mentioned. But give me more time to think about that issue. Perhaps that is just a small issue.

If it's a bigger issue, just allow the python executable to be easily selected (or maybe use python2 if that works on other platforms) and for now it would be OK. If possible (this should be easy), just print a nice error if invalid version is detected.

What in situation when in existing directory are for example header and source files for leds? Then in next step someone decide to remove it and just leaves the buttons. Then files for leds should be removed. It was easier for me to just remove whole directory and generate new files based on .json.

Hmm... good point. Trying to be smart may be pretty hard, so for now let's leave that unsolved. In the future we may come up with something (I have a couple of ideas). I think that this is only a minor inconvenience, as with such modifications the LEDs would be disabled by Kconfig anyway, so nothing would be actually compiled (the contents of the files are surrounded with #ifdef ENABLED ... #endif). That's why we can solve that later.

But in situation when we need to generate whole new folder for board with the second argument of script? Then .json file should be also copied to this folder?

No need to copy the .json file - just leave that to the end user.

Now BOARD_NAME is used to replace this string to specified in .json board name in my script (sorry for that :)). Furthermore, BOARD_NAME-leds.cpp.jinja as a prefix suggest that the output file should be look in some similar way (with some prefix). File name leds.cpp.jinja suggests that output file should be leds.cpp without prefix.

If you have any use for that prefix, then let's keep it.

CezaryGapinski commented 7 years ago

Hi!

Can you help me with this commit?

  1. You should add board target to SIMPLE_TARGETS in Makefile - otherwise your script cannot be used when distortos is not configured. Try to do make distclean + make board ... - it will fail exactly because of that.

Fixed

  1. I think that the help text is misleading - square brackets suggest that the arguments are optional, while they are not.

Can you help me with this. I'm not sure where square brackets should be. make configure also has square brackets, but argument is not optional. Perhaps I missed something.

  1. I also think that you should not use CONFIG_BOARD as the name of one argument - this variable is actually provided by distortosConfiguration.mk, so if you omit that argument (and when distortos is configured), you get:

    $ make board OUTPUT_BOARD=xxx python scripts/generateBoard.py -c "32F429IDISCOVERY" -o xxx Traceback (most recent call last): File "scripts/generateBoard.py", line 222, in if name == 'main': main() File "scripts/generateBoard.py", line 174, in main with open(inputParameters.configFile) as configDataFile: FileNotFoundError: [Errno 2] No such file or directory: '32F429IDISCOVERY' make: *** [Makefile:283: board] Error 1

    Probably not what you want (; Maybe CONFIG_FILE? Then maybe OUTPUT_PATH instead of OUTPUT_BOARD?

Ok, now is CONFIG_FILE and OUTPUT_PATH.

  1. If I force use of Python 2 in the Makefile, it seems to work correctly. How hard would it be to make the script compatible with Python 2 and 3 at the same time? Not necessarily now, but in the future? For now if you know of no other way (would using python2 be generic and work on "all" platforms?), I suggest the make board to use $(PYTHON) instead of python, so that I can override that in the invocation. And maybe print some informative error ("This script currently requires Python 2, try make board PYTHON=pytnon2 ..." or sth like that) so that it would be obvious what is going on.

Can you look if this:

ifeq "3.0.0" "$(word 1, $(sort 3.0.0 $(word 2,$(shell $(PYTHON) --version 2>&1))))" 
  @echo 'This script currently requires Python 2, try:' 
  @echo 'make board PYTHON=python2 CONFIG_FILE=<file_path> [OUTPUT_PATH=<directory_path>]' 
else

is sufficient?

  1. I see that the script completely removes the path that is generated. After second thought I think it should be changed. At least because this would allow the .json files to be stored in a more natural location - source/board/... (along with the generated files) instead of configurations/.... For now it would be OK to have the script only overwrite the files it generates. Sorry if I suggested something opposite earlier... Using such location would also allow the second script argument to be optional - if no output path is given, the board is generated in the same folder as the one with .json file.

Ok, now the output argument is optional, but I'm not sure if my condition in code for that is ok:

ifdef OUTPUT_PATH 
    $(PYTHON) $(DISTORTOS_PATH)scripts/generateBoard.py -c $(CONFIG_FILE) -o $(OUTPUT_PATH) 
else 
    $(PYTHON) $(DISTORTOS_PATH)scripts/generateBoard.py -c $(CONFIG_FILE) 
endif 

Perhaps exist an easier way to distinguish if there is optional argument or not?

FreddieChopin commented 7 years ago

Can you help me with this. I'm not sure where square brackets should be. make configure also has square brackets, but argument is not optional. Perhaps I missed something.

The argument for make configure is in fact optional. If you don't provide it, the file distortosConfiguration.mk is expected in the "current directory", so make configure is equivalent to make configure CONFIG_PATH=..

From what I see in the commit, the message is OK now (;

Can you look if this: ... is sufficient?

I think it's OK. You may want to consider replacing @echo ... with $(error ...), but in that case you'd have to put both strings into one "command". So $(warning ...) is also an option. Or some combination of both. Or leave that as is (;

Perhaps exist an easier way to distinguish if there is optional argument or not?

As you see with make configure it's easier, as the configure.sh script just takes positional argument, without any leading -<char> flags. In that case I just put $(CONFIG_PATH) there and it either is a string or it's "nothing" - both are perfectly fine. But if the script is more complicated and has many optional functionalities, then this becomes much harder. From what I see, your script has one required argument (config file) and two optional arguments (output path and search path). I'm not sure whether search path is useful or not (I see no use for that now, but I may have missed something), but if you'd drop it, then you could use positional arguments only - first (required) is the script, second (optional) is the output path.

If the option to use only positional arguments for your script is not feasible, then you may leave that as is, or - if it really bothers you - do something like this:

ifdef OUTPUT_PATH
    $(eval OPTIONAL_OUTPUT_PATH := -o $(OUTPUT_PATH))
endif
$(PYTHON) $(DISTORTOS_PATH)scripts/generateBoard.py -c $(CONFIG_FILE) $(OPTIONAL_OUTPUT_PATH)

Whether or not this is "better" is probably a matter of taste.

CezaryGapinski commented 7 years ago

Can you help me with this. I'm not sure where square brackets should be. make configure also has square brackets, but argument is not optional. Perhaps I missed something.

The argument for make configure is in fact optional. If you don't provide it, the file distortosConfiguration.mk is expected in the "current directory", so make configure is equivalent to make configure CONFIG_PATH=..

From what I see in the commit, the message is OK now (;

Ok, thanks for the explanation.

Can you look if this: ... is sufficient?

I think it's OK. You may want to consider replacing @echo ... with $(error ...), but in that case you'd have to put both strings into one "command". So $(warning ...) is also an option. Or some combination of both. Or leave that as is (;

So I leave that as it is now.

As you see with make configure it's easier, as the configure.sh script just takes positional argument, without any leading - flags. In that case I just put $(CONFIG_PATH) there and it either is a string or it's "nothing" - both are perfectly fine. But if the script is more complicated and has many optional functionalities, then this becomes much harder. From what I see, your script has one required argument (config file) and two optional arguments (output path and search path). I'm not sure whether search path is useful or not (I see no use for that now, but I may have missed something), but if you'd drop it, then you could use positional arguments only - first (required) is the script, second (optional) is the output path.

There is a third argument where additional searching paths can be added, but it is not used right now, to simplify make board I decided to omit this argument. But I don't want to remove it completely, perhaps that would be useful for out of makefile usage of generateBoard.py.

If the option to use only positional arguments for your script is not feasible, then you may leave that as is, or - if it really bothers you - do something like this:

ifdef OUTPUT_PATH
$(eval OPTIONAL_OUTPUT_PATH := -o $(OUTPUT_PATH))
endif
$(PYTHON) $(DISTORTOS_PATH)scripts/generateBoard.py -c $(CONFIG_FILE) $(OPTIONAL_OUTPUT_PATH)

Whether or not this is "better" is probably a matter of taste.

I think that it what I missed and I have to fix it in this commit. It would be good to rename OPTIONAL_OUTPUT_PATH to OPTIONAL_BOARD_ARGUMENTS, if in the future additional -s argument would be really useful in script?

Do you have any other suggestions ;)? BTW, I've seen that you have added 32F746GDISCOVERY board. That worked quite ok, when I tried to generate these files via script ;).

FreddieChopin commented 7 years ago

I think that it what I missed and I have to fix it in this commit. It would be good to rename OPTIONAL_OUTPUT_PATH to OPTIONAL_BOARD_ARGUMENTS, if in the future additional -s argument would be really useful in script?

I don't have a strong opinion here. We can always change the name (it's internal anyway) later, so either option will work fine.

Do you have any other suggestions ;)?

Currently none, but I did not recheck your script in action yet (; I'll try to run it soon and will let you know if I find any issues.

CezaryGapinski commented 7 years ago

Ok, no problem, if you will have time just let me know, it 's really important to know your opinion. I rebased board_script branch with actual master branch and also added config to the 32F746GDISCOVERY board.

FreddieChopin commented 7 years ago

I've checked your branch and it works OK (; There's one thing I would maybe change - there's no need for boardConfiguration.json to be called that way, so I would just name each file as the board, for example STM32F4DISCOVERY.json or sth like that.

CezaryGapinski commented 7 years ago

For me the file name as the board name is more clear, so I think this is a good step to change these names. Furthermore earlier I made mistake in copy-paste to incorrect output board folders and this is a second good reason to rename :).

FreddieChopin commented 7 years ago

So should we declare your branch ready for merging? (;