Skip to content

F improvements for teensy3 1 to master #12835

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

Conversation

purdeaandrei
Copy link

Summary of changes

  • Fixes various bugs related to the TEENSY3_1 board (see individual commit messages for details)
  • Adds error message if PWM period is selected to be too high. In particular the example mbed codes show a period of 4 seconds, which is impossible with this hardware due to the limited clock divider size.
  • Enables mbed os 5 support for TEENSY3_1
  • Two small bugfixes to the K20D50M board, which I cannot test because I don't own this board, but they are trivial enough to understand from the commit and commit message.

Impact of changes

Migration actions required

Documentation

Documentation should mention that mbed os 5 is now supported for TEENSY3_1.


Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR
[x] This is a rebase of PR 12834. I have tested the changes (applied to the mbed-os-5.15 repo) on a TEENSY3_2 board (the TEENSY3_1 target is compatible with both TEENSY3_1 and TEENSY3_2, the differences are negligable). I have not tested the bugfixes to the K20D50M board because I don't have it.

Reviewers


…non-portB pins

Note: previous code here used to say:
"enable open drain for I2C pins, only port b available"
And the proceeded to strip off the Port letter, and modify
the open drain setting for a pin with the same number on PORT B.
If the pin configured as i2c wasn't on PORTB, then the previous
code was touching the open-drain settings for the wrong pin, which
is just plain wrong.

Anyway, the comment suggests that the other ports
might not have open drain operation, however a review of documentation shows:

1) For the "MK20DX128VLH5" chip used by the K20D50M board, neither
the datasheet nor the reference manual seems to say anything about inexistent
open drain capability

2) For the "MK20DX256VLH7" chip used by the TEENSY3_1 board, there
is a mention of a pin potentially not supporting open-drain, without
listing which pins are like that, however it also says that the ODE
bit would be read-only in that case. I have tested some I2C-enabled
PORTC pins on TEENSY3_1, and the bits are not read-only, and changing
them has an effect on the physical hardware.
… of the PCR register is calculated

This makes it simmetric with how it's done in pin_mode(), and saves about 2 instructions too in pin_functions().
…ng, but luckily they were never found, because they were not the first in the list with the same pin.
…n the comment, and PTD5's comment was wrong.

Note: tested on hardware: PTD4 was not useable, and now it is.
* added support for module FTM2
* added 4 missing pwm-compatible pins
* renamed PWMn keys to match documentation from https://os.mbed.com/platforms/Teensy-3-1/
    (this was done, rather then changing documentation, bedcause these pins weren't working anyway because of missing FTM2 support)
* tested on hardware
@purdeaandrei
Copy link
Author

Question: I created two PRs: one to 5.15 and one to master. Is this what I'm supposed to be doing?
Note that I have only done testing with the changes as they apply to 5.15.
The rebase was uneventful, there was only a small conflict to fix due to formatting changes in targets.json.

@ciarmcom ciarmcom requested review from maclobdell and a team April 20, 2020 19:00
@ciarmcom
Copy link
Member

@purdeaandrei, thank you for your changes.
@maclobdell @ARMmbed/mbed-os-maintainers please review.

@@ -1712,10 +1712,12 @@
"SLEEP",
"SPI",
"SPISLAVE",
"STDIO_MESSAGES"
"STDIO_MESSAGES",
"USBDEVICE"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how's enabling USBDEVICE related to Mbed OS 5 ? It should be a separate commit, also includes test logs for this in the PR itself

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's related in the sense that USB Device was already supported in Mbed OS 2, and for some reason it worked even though USBDEVICE was not declared in the .json file. If you notice, I declared USBDEVICE but I didn't have to add any .c code to support it, it's because it's all already there. However if you still think I should separate it out, I'm fine with it. Should I force-push to this branch?

Regarding test logs -- Is there some specific testing that I should do that I should read up on? I tried both USBSerial, and USBMSD with a dummy buffer, and they worked fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked ,correct was already implemented. All fine then

@mergify
Copy link

mergify bot commented Apr 22, 2020

This PR cannot be merged due to conflicts. Please rebase to resolve them.

@ghost
Copy link

ghost commented Apr 23, 2020

Teensy 3.1 was removed in #12810

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 24, 2020

Teensy 3.1 was removed in #12810

Correct. I wanted to get this in prior that PR. We need to move Teensy 3 to be custom target outside of this repository tree then.

@MarceloSalazar
Copy link

@purdeaandrei as commented, we won't support the KL20D50 family (latest PR #12864), therefore it makes sense to close this PR and add support for the TEENSY3_1 as a custom target.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants