search

terjeio / grblHAL

This repo has moved to a new home https://github.com/grblHAL
231 stars 90 forks source link

Spindle PWM driver updated #251

Closed Volksolive closed 3 years ago

Volksolive commented 3 years ago

Spindle PWM is now up and running, but you should definitely give it a try mainly for spindle EN and Dir as I don't have a machine I can test that upon (or at least not a machine that is in working condition yet). I had to modify plugins.h because there was an extern include for i2c_init() but i2c_init with small i is the SDK function and not the i2c.h function so i replaced with i2c_Init().

terjeio commented 3 years ago

Thanks. I'll merge this by hand to my subversion repo and recommit as you have removed io expander code that should stay. Please note that changes to plugin and core code (in the grbl folder) may trigger an update of all drivers and should be avoided or done in a manner that does not do so.

Volksolive commented 3 years ago

Sorry about the io expander code removal ... I know that modifying GRBL code impact all drivers but unfortunately I had no choice as I would get an error when compiling and I cannot change the SDK function ..

terjeio commented 3 years ago

I have now commited an update with your changes. I had to modify the PWM prescaler value to get 50Hz right, prescaler values should at some point be optimized for the PWM RP2040 clock frequency/PWM peripheral. Spindle on/dir output is working.

I have removed the PIO code that I used to test the stepper timer, and there is no need to clear the NVIC irq flag for it as it is cleared automatically when clearing the source?

I do not get errors when compiling on the RPI with VS Code, can you post the errors you get here?

.gitignore changes should be for the build directory, not specific files in it?

Volksolive commented 3 years ago

Yes I haven't changed the PWM prescaler because I was unsure if specific frequencies/resolution were needed there.

Regarding the NVIC irq clear, I thought it would be needed but if only clearing the PIO flag do clear the IRQ then no need to clear the NVIC flag .. but I find this quite strange, not really the way you would do it with any other peripheral right?

here is the error I get: In file included from E:\Documents\GitHub\grblHAL\drivers\RP2040\grbl/hal.h:34, [build] from E:\Documents\GitHub\grblHAL\drivers\RP2040\driver.h:34, [build] from E:\Documents\GitHub\grblHAL\drivers\RP2040\i2c.h:25, [build] from E:\Documents\GitHub\grblHAL\drivers\RP2040\i2c.c:28: [build] E:\Documents\GitHub\grblHAL\drivers\RP2040\grbl/plugins.h:187:13: error: conflicting types for 'i2c_init' [build] 187 | extern void i2c_init (void); [build] | ^~~~ [build] In file included from E:\Documents\GitHub\grblHAL\drivers\RP2040\i2c.c:26: [build] E:\Documents\Electronique\RPI_Pico\pico-sdk-master\src\rp2_common\hardware_i2c\include/hardware/i2c.h:83:6: note: previous declaration of 'i2c_init' was here [build] 83 | uint i2c_init(i2c_inst_t *i2c, uint baudrate); [build] | ^~~~ [build] NMAKE : fatal error U1077: 'D:\PROGRA~2\GNUARM~1\102020~1\bin\AR19DD~1.EXE' : return code '0x1'

you can see that i2c_init refers to the SDK hardware/i2c.h function Anyway I think it is a bug in all drivers if plugins refers to i2c_init when the actual i2c.c function is called i2c_Init with capital I letter.

no other file / folder than build in my .gitignore

terjeio commented 3 years ago

Regarding the NVIC irq clear, I thought it would be needed but if only clearing the PIO flag do clear the IRQ then no need to clear the NVIC flag .. but I find this quite strange, not really the way you would do it with any other peripheral right?

IIRC I only clear the peripheral flags not the NVIC counterpart in other drivers. Clearing the peripheral flag clears the NVIC copy but not vice-versa, at least for RP2040.

you can see that i2c_init refers to the SDK hardware/i2c.h function Anyway I think it is a bug in all drivers if plugins refers to i2c_init when the actual i2c.c function is called i2c_Init with capital I letter.

Hmm. Maybe the init prototype should be moved to the driver specific i2c.h, not changed. This since it is not called by the core and should not be called by plugins either. I'll have to revisit all drivers/plugins and perhaps add a HAL capability flag for i2c for plugins to check (if needed).

no other file / folder than build in my .gitignore

This pull request adds some explicit files in the build folder, and I see now that yout first pull request added a ton too. I'll change it to exclude the build folder instead of files in it.

Volksolive commented 3 years ago

You are totally right, I got confused with the irq_clear() function that I think serve for clearing an IRQ that has been force set. The clearing of IRQ always happen at peripheral level. What is strange for the PIO is that it is not very clear in the DS that IRQ register is use to clear the IRQ as in the list of register it is not close to other INT register and does not have a name like PIOICR, like for example the UART UARTICR Int Clear Register ..

Volksolive commented 3 years ago

Could you explained me what is the gpio mode option ?

define GPIO_SHIFT0 0

....

define GPIO_MAP 14

define GPIO_DIRECT 15

define GPIO_IOEXPAND 16

I understand what is GPIO_IOEXPAND, GPIO_MAP and DIRECT but I am not sure to understand the shift .. do we need them with the RP2040?

BTW is that the best way for us to communicate or should we discuss on another channel?

terjeio commented 3 years ago

Shift is the fastest way to read/write signals and can be used if the pins for the group are consecutive and matches the underlying structure. A shift on an ARM processor carries no overhead (or did not do so when I started coding for ARM2 many moons ago) so basically only two instructions are needed to transfer the data to/from the register and memory (if the adresses are already loaded). So not really needed - only me beeing old school preferring not to waste processor cycles...

BTW is that the best way for us to communicate or should we discuss on another channel?

I prefer most discussions to stay out in the open, especially when of relevance to others that may join in later. A better place than this is to start a new discussion when it is of general interest, or add to an ongoing if only related to that.

Other channels are an option, I am emailing and/or discussing stuff on Skype with a couple of people involved.

Volksolive commented 3 years ago

Ok understood, is the reason why shift stops at 13 is that on the ARM2 MCU the IO port would be 13 pins wide?

I was asking about other channel because me asking question like this or related to some driver specific code would not really be of interest for anybody but me. Also thinking that quick question like that would fit better on a chat like channel than a forum.

terjeio commented 3 years ago

Ok understood, is the reason why shift stops at 13 is that on the ARM2 MCU the IO port would be 13 pins wide?

No, sloppy of me - the code is inherited from another driver that has 16 bit wide GPIO registers. If to be used shifts should be added up to 29 for this processor. This since using shifts for i/o groups with less than 3 signals is not of much benefit as they are interacted with rather infrequently.