Skip to content

mimxrt: Five small service type changes. #9137

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

mimxrt: Five small service type changes. #9137

wants to merge 6 commits into from

Conversation

robert-hh
Copy link
Contributor

@robert-hh robert-hh commented Aug 29, 2022 •
edited
Loading

  1. Remove a few commented code lines from machine_uart.c
  2. Change the header of the load image to match the new teensy loader, which does not erase the file system any more, given that the header version is 4.3 and the final binary size is properly set in the header. The new Teensy loader will be installed at the device is any sketch is loaded to the Teensy device using the up-to-date teensyduino package. This change would not be needed to keep the file system if mimxrt/mboot: Adds bootloader support. #8229 was merged, which is the better solution as being uniform for all boards.
  3. Alllow the keyword option cs=-1 for the machine SPI class. In that case, the hardware CS pin will not be controlled by the SPI controller, but has to be set by the application. That allows for arbitrary pins being used for cs.
  4. Set the uart ioctl write poll flag properly. It now will be set when the send buffer is empty. Before, it always returned that writing is possible.
  5. Fix a regression bug in uart.write() causing data to be sent incomplete.

@robert-hh
mimxrt: Drop a few commented lines in machine_uart.c.
e5f7ea6
@robert-hh
mimxrt: Format the firmware image to match the new teensy loader.
2fda243 
The new teensy loader keeps the file system under certain conditions:

- The file size is properly set in the file header.
- The header version is 4.3

These changes are implemented here, requiring a backport of
fsl_flexspi_nor_boot.c.
There is still a problem with the command line version of the teensy
loader, which fails on the first attempt. At the second attempt it
works. The GUI version of the teensy loader is fine.
@robert-hh
mimxrt: Allow a setting of -1 for cs in the constructor.
d5ea1d5 
In that case, no Pin will be configured for the CS signal, even if
it is internally still generated. That setting allows to use any pin for
CS, which then must be controlled by the Python script.
@robert-hh
mimxrt: Set the uart ioctl write poll flag properly.
6730c39 
It was always set to True. The change adds a check to the tx
status flag which is set when all data is transferred.
@robert-hh
Copy link
Contributor Author

@dpgeorge The recent commit contains the changes you suggested. While looking at the machine_uart.c code and having the discussion #9187 in mind, I noticed that the timeout handling in uart.write() is different. It does not care for the timeout set at the object, but calculates a timeout from the data size and the transfer speed. To make it consistent, that should be changed. So I'll make another commit in this PR.

@robert-hh
mimxrt: Implement the review comments.
a77a47a
@robert-hh
mimxrt: Fix a bug in uart.write().
e435583 
Causing an incomplete send if the data size was longer than the
buffer size.
@robert-hh
Copy link
Contributor Author

Changing the timeout mechanism requires a complete rework of uart.write, to work with ring buffers. So for now I just added a bug fix. That bug causes data to be sent incomplete, if the data size was larger than the buffer size. I'm pretty sure that I tested this case before, but there was a change in the library version.

@robert-hh
Copy link
Contributor Author

@dpgeorge About discussion https://github.com/orgs/micropython/discussions/9187, I could add a check in uart.write(), that in case the timeout is set to 0 and the data cannot be delivered immediatly, it returns a timeout error with 0 data being delivered. Would that properly interact with the stream writer mechanism?

@robert-hh robert-hh changed the title mimxrt: Four small service type changes. mimxrt: Five small service type changes. Sep 9, 2022
@dpgeorge
Copy link
Member

I could add a check in uart.write(), that in case the timeout is set to 0 and the data cannot be delivered immediatly, it returns a timeout error with 0 data being delivered.

Definitely the UART should support non-blocking mode, which is selected by timeout=0 in the constructor/init method. For the details of what/when to return see the stm32 and rp2 implementations of UART, they should be doing it correctly (I think it should return MP_EAGAIN if no data could be sent, and the total number of bytes if any data could be sent).

@robert-hh
Copy link
Contributor Author

robert-hh commented Sep 11, 2022 •
edited
Loading

I implemented as a test a method which checks, if the data can be sent without blocking for longer than the set timeout, which can be 0. And yes, both rp2 and STM32 set MP_EAGAIN and return MP_STREAM_ERROR in case of a timeout.
So I'll add that as another commit.

@robert-hh
Copy link
Contributor Author

@dpgeorge One question: if let's say:
txbuf is set to 256, timeout is set to 0, message size is 400. Should the code:
a) return 0, because it cannot send 400 bytes without blocking, or
b) send 256 bytes and return 256.
I assume the proper answer is b)

@dpgeorge
Copy link
Member

It should be (b). That's how TCP sockets work, and why Stream.drain will continue to call the underlying stream's write method until all data goes out.

@robert-hh robert-hh changed the title mimxrt: Five small service type changes. mimxrt: Six small service type changes. Sep 11, 2022
@dpgeorge
Copy link
Member

@robert-hh let me know when this is ready to review.

@robert-hh
Copy link
Contributor Author

I usually push things only when they are ready to review, especially for branches where I already made a PR. For the latter ones I mostly make commits to keep them compatible with the evolving master branch.

In this actual PR I was anxious that you would have merged it already, because I'm not happy with the last commit. I works as intended, but the code style is nasty. So i can have another look at it now.

@robert-hh
Copy link
Contributor Author

@dpgeorge It's ready now. I could not do much about it that removing one obsolete variable and renaming another one. And adding a few more comments.

I have a similar change already for rp2, but that one simplifies the existing code (based on the SAMD PR). But instead of making a PR for every port, I will go through the other ports and see, If I can make a similar change there and have a combined PR, like the uart.flush() series. I already noticed that the ESP32 call for uart_write_bytes() does not work as documented. The documentation tells that the call does NOT block, but it does, and the code is implemented as blocking.

@robert-hh robert-hh force-pushed the mimxrt_sp8 branch 2 times, most recently from 297a702 to e435583 Compare September 12, 2022 15:27
@robert-hh
Copy link
Contributor Author

Hello @dpgeorge I removed the last commit and placed it into another PR candidate, which addresses the UART write timeout for various boards. That's more consistent.

@robert-hh robert-hh changed the title mimxrt: Six small service type changes. mimxrt: Five small service type changes. Sep 13, 2022
@dpgeorge
Copy link
Member

Rebased and merged in 65ce7d7 through 6472348

@dpgeorge dpgeorge closed this Sep 13, 2022
@dpgeorge
Copy link
Member

In this actual PR I was anxious that you would have merged it already

If you want to put the PR on hold, convert it to a draft (there is a setting somewhere on the PR page to do that).

@robert-hh robert-hh deleted the mimxrt_sp8 branch September 13, 2022 08:54
robert-hh added a commit to robert-hh/micropython that referenced this pull request May 29, 2023
@robert-hh
mimxrt/fsl_flexspi_nor_boot.c: Use a specific version for mimxrt1062.
4276f7d 
This one sets the flash imange length properly for the teensy loader,
such that the file system is not erased. It was already set in
PR micropython#9137, but got lost when the MIMXRT1176 board was added.

Signed-off-by: robert-hh <robert@hammelrath.com>
RetiredWizard pushed a commit to RetiredWizard/micropython that referenced this pull request Apr 9, 2024
@tannewt
Merge pull request micropython#9137 from dhalbert/doc-espressif-analo…
6e5a875 
…gin-limitations

document Espressif AnalogIn limitations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpgeorge dpgeorge dpgeorge left review comments

Assignees
No one assigned
Labels
port-mimxrt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@robert-hh @dpgeorge