Skip to content
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

Fixes for issues in #3648 #3652

Merged
merged 4 commits into from
May 21, 2024
Merged

Conversation

jmattsson
Copy link
Member

Fixes #3648.

  • This PR is for the dev-esp32 branch rather than for the release branch.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/*.

It was pointed out that the switch to use the IDF's stdin inadvertently broke some IDE's due to different CR/LF handling, as well as breaking uart.on('data') handling on the console uart. This PR addresses both. The first by changing the default IDF CR/LF handling to do no translation which should match what we used to do, and the second by refactoring the uart.on('data') handling and hooking it into the new console task in addition to the uart task. In the future, we should probably consider adding a console.on('data') module/function to allow this sort of functionality also on models which use CDC or USB-Serial-JTAG consoles (at which point the uart.on('data') should just be a forward to that, for the console uart).

Additionally I've added node.chipmodel() to allow querying the model of ESP32 that NodeMCU is running on.

I've tested this on ESP32-C3 as that was what I had on my desk at the moment, and as far as I can tell it now works correctly. CR/LF handling is madly confusing with local terminal line discipline, ttyUSB line discipline, and ESP internal CR/LF translation all acting together and causing confusion.

@serg3295 could you please take this for a spin? I do not have VS Code to test with, and I can't find an ESPlorer version that supports io.open() instead of the old file.open() to test file uploads there.

Jade Mattsson added 3 commits May 12, 2024 20:22
With the switch to use the IDF's stdin for feeding the Lua VM, we
unintentionally lost the ability to use uart.on('data') on the console uart.
This is since we no longer install the nodemcu uart driver on said uart.
In order to resolve this shortcoming, this commit refactors the uart.on('data')
delimiter handling and moves it away from platform.c into uart.c where it
really belongs. A new function, uart_feed_data(), is introduced, which is used
both by the nodemcu uart driver task as well as the nodemcu console driver
task (assuming the console is in fact a uart).

The linebuffer allocation/freeing is still in response to
uart.start()/uart.stop(), but it is now in uart.c rather than
platform.c.

The whole uart integration is still too tightly coupled between the platform
component and the module component's uart.c, but this makes it slightly
better at least.
With the change to use the IDF's stdin to feed the Lua VM, the default
IDF CR/LF translation appears to cause issues with some IDEs.
Updated and removed incorrect node module documentation.
@serg3295
Copy link

Thanks for the PR!

a uart.on() callback handler works now. But uart.start()/uart.stop() are mandatory.
The test code that works:

uart.start(0)
uart.on(0,"data",3,function(d)
  uart.write(0,"test\n")
  uart.on(0,"data")
  uart.stop(0)
  end, 0)
uart.write(0,"Ready\n")

ESPlorer is not working. I guess, because it doesn't send uart.start|stop.

I will try to figure out the CR/LF handling.

@serg3295
Copy link

I fixed nodemcu-tools (VSCode extension). Now it works with this PR.
nodemcu-tool (CLI utility) also works for both ESP8266 and ESP32.

However, the issue of determining the architecture of the esp chip remained unresolved when launching the nodemcu-tool and nodemcu-tools applications. This is required for further correct operation of applications.

The following functions can be used to determine:

function esp8266 esp32 esp32xx
node.info() + - -
node.chipid() + + -
node.chipmodel() - + +

From the table above, it can be seen that none of the functions currently allows you to determine all possible types of chips at once with one command.

If node.chipid() will return string "0x_unknown" for esp32xx chips (PR #3651), then Lua will not throw an error. Handling this exception is a non-trivial task in vscode-nodemcu-tools due to the complexity of the internal protocol.
This is much easier to do in nodemcu-tool.

What's wrong with returning such a string?
We are not deceiving the user. The hexadecimal digit is really unknown. :-)

ChipID: 0x_unknown will be output instead of ChipID: 0x837c81111119

❯ ./nodemcu-tool fsinfo
[NodeMCU-Tool]~ Connected 
[device]      ~ Arch: esp32 | Version: unknown | ChipID: 0x837c81111119 | FlashID: 0x_unknown 
[device]      ~ Free Disk Space: 403 KB | Total: 404 KB | 1 Files 
[device]      ~ Files stored into Flash (SPIFFS) 
[device]      ~  - wifi32start.lua (321 Bytes) 
[NodeMCU-Tool]~ disconnecting 

The new node.chipmodel() function will allow a more accurate display of the esp 32 model after the architecture is defined.

chipRender

@marcelstoer
Copy link
Member

@serg3295 Thanks for testing these combinations! Would you maybe have the bandwidth to address #2759? It would allow us to add a few more '+' to your table above 😄

@serg3295
Copy link

@serg3295 Thanks for testing these combinations! Would you maybe have the bandwidth to address #2759? It would allow us to add a few more '+' to your table above 😄

I'll try, but I don't know C very well.
I'm afraid that my code will need to be heavily corrected 😄

@serg3295
Copy link

serg3295 commented May 15, 2024

another problem.
I send binary file to ESP with the contents
ok

ESP replaces all 0D with 0A. I think it's a bug.

resulting file on SPIFFS

err

The code of receiver:

__f = io.open("crlf.bin", "w")
uart.start(0)
uart.on("data", 10, function(d)
  __f:write(d)
  uart.on("data")
  __f:close()
  __f = nil
  uart.stop(0)
end, 0)

@serg3295
Copy link

I suggest revert commit to default

  • Line ending for UART output (CRLF)
  • Line ending for UART input (CR)

because some log messages will be displayed more correctly in IDEs.

All IDEs works fine with these CRLF/CR settings.

@jmattsson
Copy link
Member Author

@serg3295 Is there something wrong with using e.g. local model = node.chipmodel and node.chipmodel() or node.chipid() to avoid errors being thrown?

As for the CR+LF to LF issue, that is (part of) what this PR is intended to fix. As I just mentioned over on the issue itself, you may not have gotten the fix activated as I only updated sdkconfig.defaults. If you do make menuconfig and then go into Component config > Newlib and ensure that at the very least the input setting is set to LF only, you should not suffer this translation issue.

Having the output direction also configured to LF fixes the issue with a double-prompt which has been present ever since we switched to using the IDF's console driver. I wasn't aware there were logging issues with fixes that. Do you have more detailed information please?

@jmattsson
Copy link
Member Author

Also, uart.start() used to be mandatory before too, that much has not changed. I also have conflicting advice on ESPlorer now - over on the issue report a have a success report with this PR.

@serg3295
Copy link

serg3295 commented May 19, 2024

If you do make menuconfig and then go into Component config > Newlib and ensure that at the very least the input setting is set to LF only, you should not suffer this translation issue.

I have tried both cr lf/cr and lf/lf. In any case, 10 is returned.

I run the test code in the terminal:

uart.start(0)uart.on("data",3,function(d)for i=1,#d do ch=tostring(d:sub(i,i))print('ch:',string.byte(ch))end uart.on("data")uart.stop(0)end,0)

then type 111

I ran the program in debugger. In uart_feed_data function

 need_len: 3
 buf: 49 //'1'

another test case:
type <cr><cr><cr>

need_len: 3
 buf: 10 

I wasn't aware there were logging issues with fixes that. Do you have more detailed information please?

There is no problem. It's just that the message is displayed more correctly.
pay attention to the line:
main_task: Started...

crlf_cr:
crlf_cr

lf_lf
lf_lf

@jmattsson
Copy link
Member Author

Huh. That's odd. I'll see if I can get some more time to do a deep-dive into the IDF internals then and figure out what's going on...

@serg3295
Copy link

Is there something wrong with using e.g. local model = node.chipmodel and node.chipmodel() or node.chipid() to avoid errors being thrown?

You are absolutely right. I don't even understand why this obvious option didn't occur to me :-/

@serg3295
Copy link

I've made small changes.

diff --git a/components/base_nodemcu/user_main.c b/components/base_nodemcu/user_main.c
index c72cc654..60ddc0bb 100644
--- a/components/base_nodemcu/user_main.c
+++ b/components/base_nodemcu/user_main.c
@@ -214,7 +214,7 @@ static void console_init(void)
   /* Based on console/advanced example */
 
   esp_vfs_dev_uart_port_set_rx_line_endings(
-    CONFIG_ESP_CONSOLE_UART_NUM, ESP_LINE_ENDINGS_CR);
+    CONFIG_ESP_CONSOLE_UART_NUM, ESP_LINE_ENDINGS_LF);
   esp_vfs_dev_uart_port_set_tx_line_endings(
     CONFIG_ESP_CONSOLE_UART_NUM, ESP_LINE_ENDINGS_CRLF);
 

Now the applications are working fine.

@jmattsson
Copy link
Member Author

@serg3295 Oh gosh, those should be using the correct macros from Kconfig, not be hard-coded! Cut-n-paste fail on my behalf, good catch, will fix!

@serg3295
Copy link

serg3295 commented May 20, 2024

@jmattsson I fixed nodemcu-tool and nodemcu-tools and tested them with settings:
rx: ESP_LINE_ENDINGS_LF
tx: ESP_LINE_ENDINGS_CRLF

I also tested ESPlorer a bit. Everything works.
I believe that setting for tx should stay crlf.

EDIT.
Hmm. Esplorer works with tx: ESP_LINE_ENDINGS_LF

I will try to fix nodemcu-tool(s) to get only LF and not expect CRLF, if it needs to be done for any reason.

@jmattsson
Copy link
Member Author

@serg3295 Okay, I've made the necessary changes to actually honour the Kconfig settings. Let me know if that looks good on your end, please?

@serg3295
Copy link

I have corrected the code of nodemcu-tool/nodemcu-tools so that they also work with the LF/LF settings.
The changes for the Kconfig settings also work for me.

@jmattsson
Copy link
Member Author

@marcelstoer since it looks like Philip is busy, and there's been external review and feedback and fixing, any objections to me merging this?

Copy link
Member

@marcelstoer marcelstoer left a comment

Choose a reason for hiding this comment

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

Thanks Jade and @serg3295 for fixing this!

@jmattsson jmattsson merged commit 918f753 into nodemcu:dev-esp32 May 21, 2024
24 checks passed
@jmattsson jmattsson deleted the fixes-for-issue3648 branch May 21, 2024 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants