search

iNavFlight / inav

INAV: Navigation-enabled flight control software
https://inavflight.github.io
GNU General Public License v3.0
3.06k stars 1.45k forks source link

STM32F7 code review request #343

Closed npsm closed 7 years ago

npsm commented 8 years ago

I've been working on F7 (and stm cube hal driver) support. I now reached a point to catch up with the master. I'll have to do some work on the merge so that might be a good opportunity to fix some ugly stuff as well. You can find the code here: https://github.com/npsm/inav

Not all drivers have been tested on hardware as I'm still waiting on some hardware.

Collaborating on a project like this is new to me so I could also use some advise on a way of working to get easy to merge pull requests.

martinbudden commented 8 years ago

@npsm ,

one thing I would advise is to do rebases against the master frequently, at least every two or three days. This may seem a pain, but lots of small rebases are much easier than one big one. It also means you see changes to the master as they happen.

Depending on how you look at it, you have either chosen a bad time to start this work, or a good time. The reason being is that there are extensive changes being made to the device drivers, to use the new IO system. The bad thing about this is that your changes become quickly out of date, the good thing is that using the new IO system should make the F7 port much easier.

The other thing I advise is a "deep then wide" approach to development. That is rather than try and do everything at once (a "wide" approach), you should do one thing deeply and thoroughly understand it, and have it reviewed before moving onto the next thing.

I've only had a cursory look at your code, but it looks like you are using the HAL to implement the F7 port. I haven't thought this through in detail, but I think the port can be done by mainly adding F7 code into the the IO (that is, drivers/io.c and related files) and a small number of other changes. This would be a different approach to the one you have taken. @ledvinap do you have a view on this?

The other thing, is the large size of all your changes makes them very difficult to review.

npsm commented 8 years ago

@martinbudden , Thanks for you your, comment. As I was eager to get something working I had to add/change quit and I'm not that disciplined in committing each small change. Now that I have a working base this will be a lot easier. Once I've catched up I will try to make more isolated updates that will be easier to intergrate.

The choice for the HAL was not really a choice to make. STM has and will not release std peripheral drivers for the F7 series. It didn't feel rigth to port the std peripheral drivers, I makes more sense to eventually move to the HAL drivers. The HAL gives better abstraction and should result in the same code for all series (F1-F7). This could remove a lot of the ifdef stuff and unify the code.

For now I will keep this as a parallel development until all drivers are tested. Important for me is that I don't stray of to far so it will never get merged into the base. This is my major concern.

ledvinap commented 8 years ago

@martinbudden : drivers/io.h was designed to hide implementation details as much as possible. Pin is exported as tag or opaque handle (both may be checked in Boolean context). Storing HAL pointers and indices in IO structure should be quite easy. Current io.c code infers lot of required values from port and pin index, but (long and repetitive) tables may be used to do this generally if necessary. Then there is ioConfig_t - it should be copyable type that describes pin configuration (basic type or structure). IOCFG_ macros must be supplied to configure IO into sensible state (this currently hides lots of F1/F3 differences)

@npsm: The are high-performance LL drives for hardware abstraction. Maybe they would match CF internals better ..

HAL is probably too memory hungry to support CC3D..

martinbudden commented 8 years ago

@npsm , re-reading it, my comment about the HAL is not that clear.

I'm not advocating not using the HAL - as you point out it is needed for the F7 port. What I was saying is that it should be used indirectly via the new IO. That is, as far as possible, F7 HAL related code should be drivers/io.c et al, and that direct calls to the HAL in device driver should be very limited.

To give an example, your drivers/led_light.c https://github.com/npsm/inav/blob/dev_F7_support/src/main/drivers/light_led_hal.c makes direct calls to the HAL,

Whereas the iNav led driver https://github.com/iNavFlight/inav/blob/master/src/main/drivers/light_led.c which has been converted to the new IO call IOConfigGPIO and the processor specific code is in IOConfigGPIO.

You will notice that, under the new IO, led_light_stmf10x.c and led_light_stmf30x.c have gone, replaced with the single led_light.c.

npsm commented 8 years ago

@martinbudden , thanks for clearing that up. My base is a few weeks old and didn't have the new IO yet. The new IO improves abstraction a lot and will need to do some work on that.

npsm commented 8 years ago

@ledvinap Your comment isn't entirely clear to me. You mention the need to store HAL pointer for io, but this is not needed. Some feature like GPIO Clocks Reset can be used without a handle. Other drivers do need them but they can be stored within the scope of the specific driver. I have done this for both spi and i2c, maybe you can have look at those.

https://github.com/npsm/inav/blob/dev_F7_support/src/main/drivers/bus_i2c_hal.c https://github.com/npsm/inav/blob/dev_F7_support/src/main/drivers/bus_spi_hal.c

These don't use the ne IO yet the pwmout, led light and beeper do. To get the new IO working I only needed to add support for clk and reset management to make it compatible. These are placed in target/stm32f7xx_hal_compat.c/h

Most code is now compatible with the base the exceptions are:

One important feature I'm missing is the EXTI, I've read somewhere that this is or will be changed as well, what will be the best startingpoint for this?

Once I get everything tested I will build it for an CC3D and see what the differences are.

ledvinap commented 8 years ago

It is possible (and hopefully easy) to store HAL handle in IO code if needed ...

I have used IO abstraction for other drivers in my local branch (https://github.com/ledvinap/cleanflight/blob/local/src/main/drivers/bus_i2c_stm32f10x.c#L349), but it is currently too far behind. I plan to merge it into CF, but basic IO+EXTI is not merged yet (after eight months)

ledvinap commented 8 years ago

EXTI is part of https://github.com/cleanflight/cleanflight/pull/1984 ..

martinbudden commented 8 years ago

@npsm , I'm gradually moving the new IO over from betaflight for the F4 port, see PR https://github.com/iNavFlight/inav/issues/338

I haven't worked on this for the past few days, but hope to find some time to do some more next week. And yes, EXTI, is on the list.

The (eventual) intention is that the device drivers for betaflight, iNav and Cleanflight are identical, but it will take a while to get there. Once I've finished this PR, the betaflight and iNav device drivers should be the same (or at least very nearly the same). So betaflight is a good reference for any work you do.

If we get this right we could end up with F7 ports for betaflight and Cleanflight "for free".

martinbudden commented 7 years ago

@npsm , I'm in the process of going through all the iNav issues and I've just come across this. To be honest, I'd forgotten all about it.

How is it going? Are you making good progress?

npsm commented 7 years ago

@martinbudden , all basic stuff is working and it should be able to fly but didn't test this yet, I've had other priorities lately, @sambas has been working on this as well and made some good progress: https://github.com/sambas/cleanflight/tree/betaflightF7

Items implmented and tested are:

Not implemented:

Known issues:

There is probably a lot more missing on this list that is not implemented and or tested. For some feature I don't have the hardware.

It won't be until end of next week before I have time again to proceed with this. I could start with making pull request for STM HAL drivers and the tested features and make a target with those.

martinbudden commented 7 years ago

Looks like you are almost there. So we'll probably be able to target this for the 1.3 version of iNav