AdaCore / Ada_Drivers_Library

Ada source code and complete sample GNAT projects for selected bare-board platforms supported by GNAT.
BSD 3-Clause "New" or "Revised" License
240 stars 141 forks source link

FE310 Time #255

Closed elbric0 closed 6 years ago

elbric0 commented 6 years ago

My initial idea was simple. I wanted to replace the Busy_Loop in the FE310 example with a Delay procedure that wouldn't require guessing the number of loops each time a new delay is required. I also wanted a that the delay wouldn't be extended if interrupts occur during the delay. So the delay had to rely on an hardware timer.

At first I was going to use the RTC. I almost hat it working, but I was worried of problems that could occur if someone change the RTC count value to correspond to the calendar time. Then I discovered the Machine Timer which is more appropriate to measure delays because it starts at zero when the MCU boots ant it's count shouldn't be changed by the program. Also, it has 64 bits ans is incremented at 32 kHz, so there is no risk of overflow even if a system remains powered for many years.

When I tested the Delay_XX Procedures based on the Machine Timer, I observed the first delay was always longer than the others. I found it was because the instructions were not yet loaded in the instruction cache and that the difference of execution time can be reduced by increasing the SPI bus frequency. While I was investigating this problem, I found that the HiFive1 crystal was not used and that the board was running on the less accurate internal oscillator. I decided to add support for clock configuration.

So, in addition to the FE310.Time package which contains the Delay_XX procedures, this pull requests contains support the following FE310 peripherals: RTC, Machine Timer, OTP memory and Clock configuration.

Fabien-Chouteau commented 6 years ago

Hi @elbric0, thanks for this great contribution!

I will get back with some more comments as soon as possible but I already notice that you have a "merge branch" commit in the pull request.

Can you try to remove it wit:

$ git pull --rebase https://github.com/AdaCore/Ada_Drivers_Library.git master

Thanks,

elbric0 commented 6 years ago

Hi Fabien,

I am sorry. I thought it was a good practice to merge the changes from the upstream repository before submitting a pull request.

I was able to remove the merge commit from my local repository with the command you suggested. However, I am not able to push the change to my github repository. git says it can't do it because the tip of my local branch is behind the tip of my remote branch. Is there anything else I can try?

Fabien-Chouteau commented 6 years ago

I am sorry. I thought it was a good practice to merge the changes from the upstream repository before submitting a pull request.

It's good that you apply your patches on the upstream repo, but there are different ways of doing this, as explained here: https://www.derekgourlay.com/blog/git-when-to-merge-vs-when-to-rebase/

We typically want to avoid merge commit in the pull requests.

I was able to remove the merge commit from my local repository with the command you suggested. However, I am not able to push the change to my github repository. git says it can't do it because the tip of my local branch is behind the tip of my remote branch. Is there anything else I can try?

The git pull --rebase changed the history of the branch (by removing the merge commit), so you have to use the force option: git push -f.

elbric0 commented 6 years ago

OK. I will know for next time.

I was able to remove the merge commit from my pull request with the force option.

CLAassistant commented 6 years ago

CLA assistant check
All committers have signed the CLA.

Fabien-Chouteau commented 6 years ago

Thanks a lot @elbric0, there's one or two comment box missing but I will fix them in my next PR.