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

Commit tool table NVS fix. #68

Closed
wants to merge 0 commits into from

Conversation

andrewmarles
Copy link

When I went to compile with the tool table enabled, initially it seemed to break the build. The changes below seemed to fix things for me and allow me to test the functionality.

This was on the teensy4.1 driver with my GRBLHAL2000 board, but I think this would apply to all drivers.

@terjeio
Copy link
Contributor

terjeio commented Sep 7, 2021

How did it break the build?

Depending on the number of tools defined this change will potentially overwrite data in the NVS_ADDR_PARAMETERS block (used for offsets). It will also break the assert in grbllib.c:

core/grbllib.c

Line 94 in 384fda6

assert(NVS_ADDR_GLOBAL + sizeof(settings_t) + NVS_CRC_BYTES < NVS_ADDR_TOOL_TABLE);

I have been thinking of adding the tool table to the end of the first 1K block in order to get more headroom. This can be done increasing the GRBL_NVS_SIZE define.

@andrewmarles
Copy link
Author

Executable went from 500k to 70k and would not run. I don’t have debugging on teensy so didn’t see where it was failing to load. I probably don’t know enough about the memory map to understand the best place to put things, still learning.

@terjeio
Copy link
Contributor

terjeio commented Sep 7, 2021

What did you set N_TOOLS to?

@andrewmarles
Copy link
Author

Left it at 8 for now.

@terjeio
Copy link
Contributor

terjeio commented Sep 7, 2021

N_TOOLS 8 lead to a failed build with the original code? It works ok for me:

Memory Usage on Teensy 4.1:
  FLASH: code:159876, data:39520, headers:8472   free for files:7918596
   RAM1: variables:131776, code:157256, padding:6584   free for local variables:228672
   RAM2: variables:12384  free for malloc/new:511904

Without tool table:

Memory Usage on Teensy 4.1:
  FLASH: code:158852, data:39336, headers:8656   free for files:7919620
   RAM1: variables:127680, code:156232, padding:7608   free for local variables:232768
   RAM2: variables:12384  free for malloc/new:511904

This is with ethernet and a few other plugins enabled. And with the 1.54 version of the Teensy libraries - earlier versions are buggy.

@terjeio
Copy link
Contributor

terjeio commented Sep 7, 2021

I've added the tool table to NVS debug output - from the $Q command. With original code and 8 tools:

[MSG:NVS Area: addr size]
[MSG:Global: 1 341]
[MSG:Tool table: 343 168]
[MSG:Parameters: 512 156]
[MSG:Startup block: 799 142]
[MSG:Build info: 942 71]
[MSG:Driver: 1024 141]

No room for expansion in the global area. This means I have to move the tool table above 1K...
Note that this is with 3 axes enabled - with four or more axes 8 tools won't fit.

With your change:

[MSG:NVS Area: addr size]
[MSG:Global: 1 341]
[MSG:Tool table: 680 168]
[MSG:Parameters: 512 156]
[MSG:Startup block: 799 142]
[MSG:Build info: 942 71]
[MSG:Driver: 1024 141]

It goes clear of the parameters area but overlaps the startup area.

Debug output has to be enabled in config.h to enable $Q.

@andrewmarles
Copy link
Author

I've closed this pull request as it obviously isn't the correct solution, but I'm going to continue looking at this because I still cannot get the tool table to compile into my build, nor is it really feasible to only have it working with 3 axes.

@terjeio
Copy link
Contributor

terjeio commented Sep 28, 2021

Can you try with this patch? It moves the tool table to 1024.

[MSG:NVS Area: addr size]
[MSG:Global: 1 341]
[MSG:Parameters: 512 156]
[MSG:Startup block: 799 142]
[MSG:Build info: 942 71]
[MSG:Tool table: 1024 168]
[MSG:Driver: 1192 91]

nvs.zip

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.

2 participants