Allow choosing FreeRTOS from STM32Cube

[g-arjones]

Closes #188

[Hish15]

Hi,

I really mean no offence, and will give you those general feedbacks :

• When yo share your branch to the world, specially when the branch is in a Pull Request, you must no longer force push to it.
  The reason is that doing so messes up, any local copy any one might have made. Plus it makes following the changes on the PR harder.
• When doing a PR, you have to try to change as little as possible in one single PR. Here you changed the whole file. It's difficult to me to see if your changes are real changes, renamed things, or things moved. The risk that something is broken is really high since you "touched everything" at once.
• More git related : Try to do more intermediate commits when it makes sense. Here you made a single commit +159 additions and −82 deletion. This is way too much (in this case). You are not suppose to make one commit per line of course, but I guess that you made all theses changes on multi steps. We want to see those steps if they help to understand.

[g-arjones]

I really did it in a single step because the change is not as big as it seems. The reason the diff is so high is because I basically moved the entire code to a macro that now gets called with different arguments depending on whether it's using a standalone FreeRTOS release or the one bundled with STM32Cube. This is actually the simplest of the three PRs that I have opened...

That said, if you feel like this is a pressing issue just let me know how you would split this in separate commits and I can do it.

[g-arjones]

Also, strongly disagree about force-pushing to a branch that is a work in progress.

You have to force push to I.e improve commit messages and split (as you have pointed out) or squash commits. Force pushing is definitely better than having a poorly written commit history and is often part of the review process. Plus, anyone expecting a pull request branch to be stable don't really understand how this works.

[Hish15]

There is no point opening an argument here, but github's documentation is cristal clear on that : No force push on PR.

Is not me being a dick, I had to open side by side your changes and the previous file to follow the changes. Your changes are not complex, but the format does not help at all the pull request process. The single commit does :

• add new vars such as FreeRTOS_FIND_COMPONENTS_PORTS
• Add family parsing
• Add a new macro
• Handle the macro call depending on the cases

Can we agree that this is a lot for one single commit?

Now that I dived in your code, I can make an actual review :

• I know that there is 0 documentation on the whole project but since you added new thing can you add some comment to help understand what the var you introduced are used for, and the macro to?
• I see no problem with the code but did you tried to setting up two devices from different families on the same CmakeList ?

Can you add the new feature to the Readme file?

[g-arjones]

There is no point opening an argument here

I won't, but you obviously missed the point about forced-pushes in Github's documentation (risk of losing commits). Your point was that it somehow "messes things up" (your words) and "makes following changes harder". My point is that fixing the commit history often does the exact opposite of that.

did you tried to setting up two devices from different families on the same CmakeList ?

Yes, this is my exact use case. That's why making FREERTOS_PATH point to FreeRTOS' source tree within STM32Cube bundle did not work for me.

Regarding documentation, I will be glad to add it as soon as I get any indication that the feature will be accepted in the first place.

[atsju]

Regarding documentation, I will be glad to add it as soon as I get any indication that the feature will be accepted in the first place.

I can give no garante here but the owner merges all PR that are judged useful and are clean. Especially if they have been discussed and tested. Here it seems that it's in real good way so I have no fear about this.

Side note about the force push : I understand your both point of view which are valid. But I tend to agree with Github documentation. Force push are really great before the PR but not once a review has been made. Because if you force push then the reviewer must remember everything or restart from zero instead just check the increment.

[Hish15]

Hi,
The changes on the readme are mandatory before merging, but, it might be interesting to add the new way to the FreeRTOS example too. It's up to you to do this now, or open an issue to do it after.

[Since there is an argument]
@g-arjones, I am a git rebase -i wizard myself to have a cleaner history but also this help my coding process a lot.
The thing is once the PR is opened things get tricky, you don't see changes as a reviewer but just something different.
If you checkout the code to test it, you will have a collision after the force push and will have to make a weird local merge (in case of collision you no longer have the same code) or a git reset --hard to the new HEAD.
So once you are sharing your branch or making a PR you must not force push. Right before merging, when all lights are green you are free to make another branch from there and use the git rebase -i Voodoo and open a new PR afterwards.

I thought I gave more explanation than "iT iS nOT GoOd", but maybe I wasn't clear enough. I hope that you see the point now that I gave you the reasons, the recommendation from github (the reason does not matter, they say no force push on PR), and If you still think that it's ok, you can do your own research. And you will see that my understanding on the topic is not off. (I did not like that part 😄)

[robamu]

Hi,

I'm interested in using this along with all other PRs by @g-arjones . I made a few smaller adaptions for the FreeRTOS file as well because the file structure of the FreeRTOS kernel has changed: #202 .
Maybe it would be a good idea to merge these two PRs. I might look into this. Just a heads-up, if this PR and my PR are merged separately, there might be a merge conflict.