Teensy-HAL #6228
Teensy-HAL #6228
Conversation
|
Overall, I would say this looks really good, nice work. We need to be very careful about the use of "#if defined (__MK64FX512__) ..." outside of the HAL itself. |
Marlin/Marlin_main.cpp
Outdated
| @@ -628,7 +628,11 @@ float cartes[XYZ] = { 0 }; | |||
| static bool send_ok[BUFSIZE]; | |||
|
|
|||
| #if HAS_SERVOS | |||
| Servo servo[NUM_SERVOS]; | |||
| #if defined(__MK64FX512__) || defined(__MK65FX1M0__) | |||
There was a problem hiding this comment.
The fundamental idea of the HAL is that all platform specific code is confined to the relevant HAL_xxx folder. The main code should not contain any references such as MK64FX512. Putting in platform specific references for every platform will make the code unmaintainable, and a lot harder to port to new targets.
This comment applies in several other places stepper.cpp, temperature.cpp
|
Yes, I'd rather not see any of the platform specific compiler directives within the code. But this would've required me to make more changes into the HAL structure and other HAL platforms and I was mainly focused on getting the Teensies up to speed and merged and not every aspect has been abstracted yet. Eventually I'd like to see each of these conditions plugged away. |
|
The HAL API will naturally evolve over time as more targets are added, which may mean some reworking of the existing HAL implementations. Unfortunately that is a "cost" of the HAL approach, but the benefit is we get a clean, maintainable and easily portable application code. The HAL_Servo should be easy to fix. The interrupt issues need investigation to see what the underlying problems is, whether it is specific to Teensy, K64, Cortex M4, ARM in general, the compiler used etc. When we understand it, then we can create a suitable abstraction. The problem with "quick fixes" is that they end up getting baked into the code, and then never get resolved. |
|
Okey I'll add the few things needed into the HAL structure. |
teemuatlut
force-pushed
the
Teensy-HAL-rebased
branch
2 times, most recently
from
f3d393d to
9014c56
Compare
|
I've now added some things into the HAL structure and you won't see any MK64FX512 ifdefs in the base code anymore. I also found a compiling issue that I can't solve. For AVR platform if you enable PINS_DEBUGGING, you're gettings errors that DIO20 and DIO21 are not declared in that scope. And I can't find where these would be, since I didn't find them in the arduino core either. But for Teensies it compiles fine with the debugging on. |
|
Looking good! |
|
There is no hardware available and I'd love to know of a Teensy -> Mega adapter shield. This was my setup some weeks ago, but I've only changed a few wires since. Much cleaner than the rats nest of jumper wires but still very much DIY. I also had to change the ramps mosfets to get them working on 3.3V. |
|
This can be merged if there's nothing further. |
|
Do you have a schematic for your setup? |
|
I have a drawn schematic that could and should be close to what I have now. You can also look at the fabulous ascii schematic in pins_TEENSY35_36.h. |
thinkyhead
force-pushed
the
32-Bit-RCBugFix-new
branch
from
dcc1ed5 to
3c33061
Compare
teemuatlut
force-pushed
the
Teensy-HAL-rebased
branch
from
9014c56 to
89b796a
Compare
|
Hi! Did you use 3.5 (which is 5v tolerant) or 3.6 version? |
|
3.5 exactly because of the tolerance. T3.6 should work as well but I have no way of testing. |
|
@thinkyhead I know it's busy times round here stabilising 1.1.x but could these changes be merged, we need to rebase 32bit again before it gets out of hand (and preferably squash ;) ) |
We have a 'Squash & Merge' button now! So... that makes it easier on you. You can keep adding commits until things are 'Right' and they can all be squished together. I'll do the Squash right now so you can start again with a clean slate. And you guys have seen this message, right? #6373 (comment) |
|
I know thinkyhead likes to oversee all things Marlin, but would it make sense for someone else to help out and adopt the 32bit effort? Is @bobc still busy with other things? |
|
Well... We both can help out on the 32-Bit stuff. And the reason I pointed you to that comment was so you know there is another path available. The entire 32-Bit code base doesn't have to be working to be merged. We can make incremental progress towards bringing it up. The big, unbreakable rule is... The 32-Bit stuff CAN NOT BREAK the 8-bit AVR stuff. But that should not be hard to accomplish with conditional compilation of the places you hook in. |
|
Oh I think you added the link after I had already read your comment. I actually think the HAL is in a pretty good condition already for a basic setup. All the AVR assembly code is conditional to 8bit and the timers and ADC are handled in the HAL. Mostly what's needed is for some brave souls to start running the 32bit branch. Even if just for 8bit AVR! The thing is that personally I have the TMC2130, TMC2208, Trinamic TRAMS, Teensy3.5, Arduino DUE and I've got an STM32 on the way, and they all are in need for testing and actually running something and I'm here sitting with only one printer. In the end it shouldn't be too hard to keep the 8bit intact. If something needs to go behind a HAL, you move the existing code to AVR HAL and specialize in your target 32bit HAL. I guess I'd just like to see the 32bit branch to be in a constant state where all that would be needed is for someone to push the merge button without a complication. It could still be running alongside the main branch but just kept up-to-date. Now this might very well be a bit harder. |
|
Just curious, why not use the IDE just compiling and uploading? For me the IDE is just a button to see what I've messed up this time but all coding is done on an external editor. @Roxy-3D The IDE compiles fine for Teensy and DUE and I believe there is even a package for STM32. One thing that could be cool is a little Marlin branded uploader tool. That's what most basic users use the IDE for anyway. |
|
@teemuatlut I don't have that hardware so it was only from what I'd seen on here, I'd happily be wrong on that. @JustAnother1 The 32bit-new branch is just a rebase, there's nothing really "new" about it other then being more uptodate and although cmake is nice using platform.io as the build system will make it so the end user doesn't need to worry about anything other than hitting compile, it sorts out the framework and libs needed, its the most Arduino-like experience (what marlin users expect) that I can find for 32bit. |
|
@Roxy-3D I have not been able to compile Matlin with make or cmake. So I either did not find the correct branch, did not manage to set it up correctly . So I#m not the one to come up with "Clear direction" for it. @p3p That it is only a rebranch doesn't help at all. The question is why do we need to have both? If there is no reason (as I assume) then please delete one of them so that others do net get confused as I'm already. I'm not too much worried about the end user, but more for the developer. It should be easy for the developer, too. @teemuatlut A few years back I looked at Arduino IDE and was stunned how basic it was. After some searching and thinking "maybe I can help them to get syntax highlighting,.. implemented I found a official Arduino statement stating that Arduino IDE was not and will never be directed to professionals. That it does not want those features that make the professional developers productive because they confuse the beginners. I understood that as direct advice. (I only call my self an professional here because I have been payed to do coding. I don't want to imply that I would be a better coder than other people using Arduino IDE) So if the decision is to base everything on Arduino IDE and to create "packages" for Arduino IDE for everything else then that might be a valid way forward, just not for me. Decisions like this are the reason for me asking for the direction of the 32bit-HAL. Again: My way is probably the wrong way. So please do what you think is the best way forward. But I think communicating these decisions / approaches / intentions would help people understand Marlin better and thereby enable them to create better pull-requests and we would end up with less dead end code,... |
|
@JustAnother1 platform.io can be used from the command line so can be used with any IDE it is developer friendly, but also has an IDE for consumers to use when they need to build and upload. I've only just started looking into it but so far I'm impressed at how streamlined it can make working with different microcontrollers. I'm just a random guy on the internet I don't get to decide what branches are removed ^^. |
|
@p3p I just wanted to make the point that having 2 branches is confusing. I don't care who deletes the branch. The decision for or against something like platform.io is something that IMHO needs to be done by the "community of developers". I would check in JTAG debug support for all the targets as something that would be more important to me than ease of use. Also performance. The stepper control has rather high requirements,... |
|
The original branch was left in place because there was a belief that some work was in progress against that branch. But other people needed the code rebased against a more current copy of RCBugFix. So that was done to help them. And when some body can further rebase against the currecnt bugfix_v1.1.0 we will probably want that done. But "Yes!" we need to clean up unused branches... |
|
bugfix-1.1.x...teemuatlut:32bit-HAL The thing is, I'm having a hard time getting this 32-Bit-RCBugFix-new branch cleanly updated to my branch. Neither rebase nor merge provided a result I was happy with. So I'd actually suggest that my working branch would supercede the two here on upstream. Any thoughts? I know this would add yet another 32bit branch but I would see it as the cleanest option and I believe it would provide a good base to gradually build upon. |
|
We could change the name of the other two 32-bit branches to have -deprecated as a suffix. At least people would know they are the 'out of favor' branches and if there is no activity for a couple of weeks, we remove them??? @thinkyhead Your thoughts? 82 files is a lot to look over... but 32-bit would make much faster progress if it was part of the main branch. Maybe the thing to do is get a few people to bring it up on 8-Bit AVR machines (Delta, Cartesian and Core ???) and if everything seems to work do the merge???? If we went this route... it might make sense to freeze all other development for a few days while we work through issues until we know everything is OK in 8-bit land. |
|
Yes, 82 is a lot. Until you start excluding the actual files that make up the HAL. Take those out, take out files that were just moved or renamed, and you end up with only 15 changed files in the main code! |
|
I'm afraid you're removing quite a few defines and abstractions in this rebase (I haven't checked all commits but specifically Rebase cleanup). the only thing I've struggled with is your modifications to pinsDebug conflicting with thinkyheads, so I've temporarily removed them from my rebase I still have a few issues but I think its relatively complete. |
|
Yeah the |
|
I'm gratified that this is being patched up and can still be made to work. It would be nice to be able to have it as a part of the 1.1.x family so that we can keep it in sync going forward. However… there's this big code reorganization that we really want to do, to break up all the gcodes into separate files and apply a proper file hierarchy. I'm pushing to restrain our further development until we can get the new file layout in place and release it as 1.2.0. And at the same time we can include the HAL as part of that initial 1.2.0 release. The thing is, this is going to take a big sprint to get in place quickly, because we don't want to sideline too many issues for too long. But I think the effort will be very much worth it. I know we're all very anxious to leave the flat file layout behind. With that in mind, we have a 3-day weekend coming up here in the States… |
|
The big reason I'm supportive of doing a new file hierarchy is because we need a place to put the 32-bit stuff and organize it. #6821 (comment) My vote is we freeze development for 2 weeks if it takes that long. But on the other side of the file hierarchy work, the 32-bit stuff has a place to continue making progress and is part of the main code base. |
|
I think it is a good idea to merge the HAL into the main dev branch, stop the constant rebasing and it will help when everyone has to write HAL compatible code, although there may be the down side that major changes will be more difficult and the HAL api is still a mess. I'm very happy to see you are going forward with the restructure, that will help everything. My plan was to get the HAL completely independent of Arduino this week after the rebase, but I guess that can wait ^^ |
|
So… I don't think the initial hierarchy work will take very long, since I've done it once in the past already, and can just follow the same pattern initially. That took a day or two last time, so I expect about the same this time. Once the new hierarchy compiles without issue, that would be the optimum time to add in the HAL. This weekend would be an opportune time to shift over to the 1.2 branch and new hierarchy, as I will have lots of space and time to dive into it. The main thing that's been holding me back from making any leap is all the issues I feel like I should read and address… |
|
Yes... But I do read just about every issue that shows up in my email Inbox. There are a few topics I just don't care about and don't read... My point is there is nothing critical going on right now. There are no emergencies cooking. If we freeze all code changes... There won't be any new emergencies happening. And people can wait until the reorganization is complete. I don't really care how the 32-bit files get organized in the new layout. But I do feel strongly we should do it as a part of the new organization. If you really can do the existing stuff in a few days, great!!! I don't care if we keep the code base frozen for an additional week to get the 32-bit stuff incorporated. But we need to get the 32-bit stuff limping along and as part of the main branch. |
|
wasn't @boelle going to try and close some abandoned issues ? |
|
@thinkyhead not sure if you want me to put the pull request in, but as I was working on this before the prior discussion. @teemuatlut it passes the Travis tests (there are some warning that need looked into) but I can't test on hardware here, I also havn't readded your debug pins code as there was considerable refactoring needed. |
|
I'm fine with whichever gets used for main branch. You too seem to have the issue that while merging into bugfix-1.1.x would be painless, getting either one of these existing 32bit branches updated would require additional work. Maybe it simple cannot be avoided. |
|
These are replacements for the current 32bit branch rather than pull requests because of the necessity of rebasing, keeping the 32bit branch "fastforwardable" from bugfix is more important than maintaining branch history. This is why the 32bit new branch exists @thinkyhead kept it rebased onto bugfix but that always breaks pull requests, usually easily sorted by just cherrypicking the commits from the pull request though. All that's a long winded way of saying I didn't expect my rebased branch to easily merge with the current 32bit branch. |
* Teensy-HAL * Pins debugger HAL * Servo library type HAL * Add _ENABLE_ISRs into HAL structure * Enable UART and stepper isr only for non 32bit platforms * Make better use of the Teensy definition macros * Fix compiling with DUE
New PR to the updated HAL.
Add Teensy3.5 and Teensy3.6 to the Marlin HAL.
https://travis-ci.org/teemuatlut/Marlin/branches Teensy-HAL-rebased
Travis is updated to run almost all of the checks on Teensy as well. I had to install a lot of utilities and libraries to get it all to compile automated. The Teensyduino from PJRC couldn't be used because of the GUI installer. The whole Travis check runs for about 14mins and should compile fine with both AVR and Teensy.
I've included some simple fixes for other platforms as well. The marlin serial had been moved to AVR HAL and the path needed an update. The Print Counter I took from the current RCBugFix.
I've made a new servo library that is inherited from the core, and just adds the few required additional methods. I saw no need to copy the whole library just to add a little something.
Added a waiting check for Marlin serial since the Teensy needed the while(!Serial) check to print properly on startup.
There was some really weird behavior that I saw with the commit that allowed UART and stepper ISRs inside the temperature ISR. The Teensy would completely Freeze and Windows wouldn't properly recognize it again. It also froze whatever serial communication program I was using at the time. I don't know why it was doing this but maybe someone else can look into it as well. I've since disabled sei and cli commands in the temp isr.
I'll try to add comments around to explain changes but I'd like to see others review and possibly even try it out, but I think this might be close to a good starting point.