-
Notifications
You must be signed in to change notification settings - Fork 17k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OS free F4Light HAL #7587
OS free F4Light HAL #7587
Conversation
Hello, Thanks for your work ! I will try to help you to split the commit in easier reviewable parts, if you agree. |
Yes thanks, but HAL_Revomini folder should be whole |
I vote fot this! merge it! The @night-ghost revo mini hal doesn't break the system and development. I don't know what is the problem to merge this hal. |
@hiro2233, that is incorrect. Blindly merging a PR that adds 177,000 lines of code across multiple libraries and multiple vehicles in one single commit without even reviewing it is luidcrous. Please don't make reckless suggestions. That is improper and makes troubleshooting nearly impossible. It is also a violation of the PR and commit guidance/standards used by ArduPilot (and basically everyone else). To clean this up into an acceptable and standard PR is not a big deal. It just needs to be broken up into several more logical commits. I think @night-ghost already gets what needs to be done, but for the benefit of @hiro2233 and anyone else... This is a lesson I learned when I first started doing PRs for ArduPilot.
This sounds like a pretty cool addition to ArduPilot. |
@hiro2233 as all submission, it need to be consistant with other works so anybody can understand it, that is why we try to unforce code style and syntax a maximun. Otherwise the project will be split under each developpers playgrounds.... Moreover the review process also permit to limit bugs and mistasks |
@night-ghost In a few hours we have the dev team weekly call. I've just added this PR as a topic to that meeting, so I believe it would be great if you could join us at http://ardupilot.org/dev/docs/ardupilot-mumble-server.html I know the time might not be perfect to you, but would be great to have you there :) Thanks, and nice job :) |
@khancyr @Pedals2Paddles yes i know that, i vote to make it splited, that was one thing that i did with URUS hal, but the diff do the work to study the changes too. Spliting for each library is reasonable, but doing that maybe doesn't make sense at all, when i started studying the ardupilot code i cloned it and studied one by one library, i didn't see the diff commit's at all. |
Definitely would be great to have you join the dev call this evening. I'm sure this will be a hot topic. |
this hal uses CMSIS, so this is a good base rtos hal too. |
sorry but no - hardly using the OS everyone understands its internal structure. And this HAL is the OS itself. So I'm fundamentally against the changes in the HAL code itself: today the last bug from the huge herd was fixed, everything works as it should, and I do not want to produce new bugs for the sake of formalities.
unfortunately, the time difference of 12 hours strongly interferes with direct communication. We have a night now. |
@night-ghost could you please look into this compiler error?
|
Shouldn't this be called ScmRtOS HAL ? |
@magicrub thats is a wiindows editors mistake encoding conversion, probably comming from a russian windows. The standard for Ardupilot should be utf-8 and thats need be added on wiki dev rules too. http://www.i18nqa.com/debug/bug-iso8859-1-vs-windows-1252.html |
@night-ghost you do not need to alter your code to split it up into multiple logical commits as described. My reply above describes the logical manner in which commits are required to be structured. Your code doesn't need to change. It just can't be all in one single commit. It only takes a few minutes in GitHub and a force push to your branch to do it. |
Three issues: |
* You may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at: | ||
* | ||
* http://www.st.com/software_license_agreement_liberty_v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pointer to the " MCD-ST Liberty SW License Agreement V2" for discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ChibiOS use CMSIS and is present on the modules, use that or in separate module for revomini hal.
On Mon, 22 Jan 2018, night-ghost wrote:
Rather simple OS-less HAL for almost any F4 board with easy portability and sub-quant scheduling accuracy.
Would be interesting to see if we could reuse the hwdef.dat we're using in
the ChibiOS port to support more than one board with this.
|
do you think that 9 assembly instructions imported from scmRTOS makes all HAL based on that OS?
AP_Math.cpp can stay unchanged, I just added long versions for some templates like constrain_value
I'm not a Git master and prefer diff/patch for internal work. So I need step-by-step instructions to do those changes.
You are wrong, CMSIS is not an OS. ChibiOS uses CMSIS too, isn't it?
I'm fundamentally against it because code generation is absolute evil. It's easier to fill the required tables and defines by hands than to understand why a super complex code generator did something wrong. For this HAL programmer should to know just Arduino's C++, without Python and generator's logic. |
How do you use Git? Is it command-line or do you have a GUI tool? I recommend SourceTree for Windows/Mac. |
Modules are "dark forest" for me :( |
command line tools scripted in .sh files for common needs. |
And few words more about dev call. The last time I heard spoken English more than 30 years ago, it's unlikely that I will be able to understand anything now. |
On Tue, 23 Jan 2018, night-ghost wrote:
Would be interesting to see if we could reuse the hwdef.dat
I'm fundamentally against it because code generation is absolute evil. It's easier to fill the required tables and defines by hands than to understand why
a super complex code generator did something wrong. For this HAL programmer should to know just Arduino's C++, without Python and generator's logic.
Yes, code generation is usually evil. But while we *do* generate C++ code
- the code being generated are static data structures.
Having sarted work on a Sparky2 port for ChibiOS, I can say that the more
we can generate from hwdef.dat the better; entering the same stuff in
several different files sucked. I believe most of the work I did there is
no longer relevant given we're sourcing more stuff from hwdef.dat
There are other things counting for using a hwdef.dat. A lot of what the
generation is avoiding is *very* knowledgeable people staring at DMA
mapping tables and trying to do the resolution in their heads. The
failures when this is done incorrectly are often surprisingly subtle.
Also - the generator is a supplied tool. Like a compiler. And not *that*
complicated :-) If it goes wrong, you can reasonably expect someone to
help fix it....
|
Found. Upstream added instantiation for long too so now there is 2 long instantiations. Will fix it |
This is not the only way! I also don't need to remember details about DMA and so on because all CPU structure is encoded in few tables. So one should only to see a diagram and say #define OSD_CS_PIN PB4 This is not much harder than your hwdef.dat but does not require superfluous entities like a generator. Porting this HAL to Cl_Racing F4 board took near half a hour - but this time includes adding support for inverted buzzer to AP_Notify |
29f7733
to
1efe215
Compare
Silence... so I think that this PR should be deleted too. |
dude.. it's 176971 lines of code. @nathantsoi joined the devcall yesterday to try and help get your branch into a mergable state because it's not even close right now. |
Even the dev team has to go to sleep at night... This is a huge add, so please give people time to review and provide input. You have people interested in this trying to work with you. No need to demand it be deleted just because it hasn't been unilaterally accepted within hours. |
this allows a HAL to have its own parameter table with parameter names generated by the build system
only for HAL_REVOMINI for now
fixes build for revomini HAL
I have merged the F4Light hal. |
finally. 👏 👏 👏 |
Thanks to all involved, but particularly Tridge and night-ghost.
It’s really pleasing that this is now in master.
For night-ghost: I assume you’ll put a post in your rc groups thread? If so, can you link it here and in discuss.ardupilot.org?
This is one of the biggest merges we’ve had, and was a massive individual contribution from you, so worthy of a blog post :)
Regards,
James
… On 10 Feb 2018, at 9:15 am, Andrew Tridgell ***@***.***> wrote:
I have merged the F4Light hal.
Many thanks to @night-ghost for your persistence with this!
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
now the time to massive changes in my fork.
it already done without me |
Thanks to @night-ghost and @tridge, I think this HAL will contribute to spread Ardupilot in low budget DIY projects and to increase the number of users a lot. |
@night-ghost @tridge @night-ghost or @anbello Would be great if any of you could create a blog post on discuss.ardupilot.org, describing the compatible boards and connections, so that we could have a wider audience that might be looking for low budget modern boards, and at the moment look for the older APM's. |
@lvale if for @night-ghost is OK I will try to do asap (even if I have problems with English language), in the meantime there is the wiki in night-ghost fork: |
@anbello Andrea, it would be excellent! |
If I clone ardupilot master and I try to build for f4light I obtain:
the problem is that in
if I rename |
I created the blog post: Please correct my English and also eventual technical errors. It will expanded in the future also, I hope, with other members contributions. |
@anbello Thanks Andrea :) Also, would be good to point to some specific boards, known/tested to work. |
do you intend more the this:
|
@anbello Great blog post, thanks!
|
ideally we should also add this board to our "Autopilot hardware Options" wiki page. |
@OlivierJB thanks for correcting my English. @rmackay9 I agree with you but I think we should wait for an official version that can be installed from the GCS. Moreover it should be explained that for some of this board is necessary a minimum soldering skill to add break out boards with baro and mag and to remove some 0 ohm resistor (jumper) in some cases. This process could be documented with a series of photos and descriptions step by step. If only I had taken some shots during the building of my quads now I would have the material for this documentation. |
Rather simple OS-less HAL for almost any F4 board with easy portability and sub-quant scheduling accuracy.
Features:
Also:
Sorry this PR don't follow rules and changes a lot of files, but it too hard to split it out. Moreover all changes outside HAL are small and straitforward.