search

chipKIT32 / chipKIT-core

Downloadable chipKIT core for use with Arduino 1.6 - 1.8+ IDE, PlatformIO, and UECIDE
http://chipkit.net/
Apache License 2.0
59 stars 53 forks source link

Remove instances of SPI define in all board variant files #144

Open EmbeddedMan opened 8 years ago

EmbeddedMan commented 8 years ago

They should all use DSPI0 now rather than the old Arduino SPI.

EmbeddedMan commented 8 years ago

I need to find more information out about exactly what this issue means. Pontech found this with their Quick240 board, and it was messing them up until they went to all DSPI definitions. So now there is no more _SPI_BASE, etc. defines in the Quick240 Board_Defs.h file. Should all chipKIT variants have all of there _SPI_BASE defines completely removed?

EmbeddedMan commented 8 years ago

Keith, Maybe you can help shed some light on this one for me. I'm not asking for you to do anything other than see if this makes sense. Pontech needed to switch over the defines in their variant file from using "SPI" to using 'DSPI0" in order to get SD card access working (I think it was SD card access) on their Quick240 board with the v1.1.0 release of core. Should this change be propagated through all other variant files? Should SPI be completely removed in favor of DSPI? Maybe that's what you've been doing already - I just want your thoughts.

EmbeddedMan commented 8 years ago

See Jacob's issue https://github.com/chipKIT32/chipKIT-core/issues/121 for other details

KeithV commented 8 years ago

Brian,

I am not sure. I don’t use, and typically have not used SPI. I think Matt at some time started to switch things over to the DSPIx #defines; I forget the reason though. I can take a look and see if I can figure it out but since I don’t use the SPI library, I am not sure exactly how that works.

KeithV

From: Brian Schmalz [mailto:notifications@github.com] Sent: Monday, January 25, 2016 4:29 PM To: chipKIT32/chipKIT-core Cc: KeithV Subject: Re: [chipKIT-core] Remove instances of SPI define in all board variant files (#144)

Keith, Maybe you can help shed some light on this one for me. I'm not asking for you to do anything other than see if this makes sense. Pontech needed to switch over the defines in their variant file from using "SPI" to using 'DSPI0" in order to get SD card access working (I think it was SD card access) on their Quick240 board with the v1.1.0 release of core. Should this change be propagated through all other variant files? Should SPI be completely removed in favor of DSPI? Maybe that's what you've been doing already - I just want your thoughts.

— Reply to this email directly or view it on GitHub https://github.com/chipKIT32/chipKIT-core/issues/144#issuecomment-174746191 .Image removed by sender.

EmbeddedMan commented 8 years ago

Keith, OK, cool. No problem. I'll investigate the matter further to try and understand where this came from, and what the ramifications would be to the other boards. Thanks for your help.

KeithV commented 8 years ago

Brian,

I took a look and I can speculate on what Matt was trying to do.

In the original variants there were things like:

That was put in to support the constructor of the Arduino SPI

For NON-PPS MCU, really only the Base was needed, and originally things like:

_SPI2_BASE_ADDRESS

Where used. But then comes PPS and we need the pins, originally the const above were used, but I think Matt noticed that we were adding pins for ALL of the DSPIx classes be it needed that for PPS as well..

But then the compiler goofed and did not add the SPI base address, and for DSPI I fixed the problem by putting in the address of the first entry in the SPI sfrs. It was an oversite in the pic32xxxx.h file to not include the base address; but I couldn’t fix that, so this is what I did.

I think more PPS parts were missing the base address in the pic32xxxx.h, so I think MATT just decided to hijack the work I did on DSPI, and in fact just used the _DSPIx_BASE and decided to do that for all boards figure I did work, so it would be right. Of course he assumed I did this for ALL boards, but in fact I only did this for Digilent boards and maybe the Fubarino’s; I did NOT do this for Pontech boards. Who knows what is in that variant.

So Matt made an assumption that what is in 1 variant is in ALL variants. That is not true, and it was untested against all of the boards he touched. The correct thing to do is for the board manufacturer, to assign the _SPI_BASE appropriately for his board; what he thinks is best. For Pontech, I would go back to the pic32xxxx.h base address define.

IMHO

KeithV

From: Brian Schmalz [mailto:notifications@github.com] Sent: Monday, January 25, 2016 4:29 PM To: chipKIT32/chipKIT-core Cc: KeithV Subject: Re: [chipKIT-core] Remove instances of SPI define in all board variant files (#144)

Keith, Maybe you can help shed some light on this one for me. I'm not asking for you to do anything other than see if this makes sense. Pontech needed to switch over the defines in their variant file from using "SPI" to using 'DSPI0" in order to get SD card access working (I think it was SD card access) on their Quick240 board with the v1.1.0 release of core. Should this change be propagated through all other variant files? Should SPI be completely removed in favor of DSPI? Maybe that's what you've been doing already - I just want your thoughts.

— Reply to this email directly or view it on GitHub https://github.com/chipKIT32/chipKIT-core/issues/144#issuecomment-174746191 .Image removed by sender.

EmbeddedMan commented 8 years ago

Keith,

For some reason, your code snippets didn't come through the GitHub site, or through my e-mail, so I'm having somewhat of a tough time following along.

I'll see if I can find out what commit (and who - I'm not sure we know who the author of these changes were) made these changes so we can go ask the author what the intent and/or thinking behind the changes is. I'd hate to make an assumption at this point before collecting as much information as possible.

*Brian

On Tue, Jan 26, 2016 at 8:20 PM, KeithV notifications@github.com wrote:

Brian,

I took a look and I can speculate on what Matt was trying to do.

In the original variants there were things like:

That was put in to support the constructor of the Arduino SPI

For NON-PPS MCU, really only the Base was needed, and originally things like:

_SPI2_BASE_ADDRESS

Where used. But then comes PPS and we need the pins, originally the const above were used, but I think Matt noticed that we were adding pins for ALL of the DSPIx classes be it needed that for PPS as well..

But then the compiler goofed and did not add the SPI base address, and for DSPI I fixed the problem by putting in the address of the first entry in the SPI sfrs. It was an oversite in the pic32xxxx.h file to not include the base address; but I couldn’t fix that, so this is what I did.

I think more PPS parts were missing the base address in the pic32xxxx.h, so I think MATT just decided to hijack the work I did on DSPI, and in fact just used the _DSPIx_BASE and decided to do that for all boards figure I did work, so it would be right. Of course he assumed I did this for ALL boards, but in fact I only did this for Digilent boards and maybe the Fubarino’s; I did NOT do this for Pontech boards. Who knows what is in that variant.

So Matt made an assumption that what is in 1 variant is in ALL variants. That is not true, and it was untested against all of the boards he touched. The correct thing to do is for the board manufacturer, to assign the _SPI_BASE appropriately for his board; what he thinks is best. For Pontech, I would go back to the pic32xxxx.h base address define.

IMHO

KeithV

From: Brian Schmalz [mailto:notifications@github.com] Sent: Monday, January 25, 2016 4:29 PM To: chipKIT32/chipKIT-core Cc: KeithV Subject: Re: [chipKIT-core] Remove instances of SPI define in all board variant files (#144)

Keith, Maybe you can help shed some light on this one for me. I'm not asking for you to do anything other than see if this makes sense. Pontech needed to switch over the defines in their variant file from using "SPI" to using 'DSPI0" in order to get SD card access working (I think it was SD card access) on their Quick240 board with the v1.1.0 release of core. Should this change be propagated through all other variant files? Should SPI be completely removed in favor of DSPI? Maybe that's what you've been doing already - I just want your thoughts.

— Reply to this email directly or view it on GitHub < https://github.com/chipKIT32/chipKIT-core/issues/144#issuecomment-174746191> .Image removed by sender.

— Reply to this email directly or view it on GitHub https://github.com/chipKIT32/chipKIT-core/issues/144#issuecomment-175347013 .

EmbeddedMan commented 8 years ago

OK, I've looked into this a bit more, and re-read the various e-mail threads over the past about changes to SPI and such. At this point in time (v1.1.0 of core) we have no libraries or any source code that needs the _SPI_BASE define and the rest of the standard Arduino SPI defines that we have in (most) varient files.

So as far as I can tell, there is no reason for us to have the old SPI defines around anymore. The SPI library that we ship only uses _DSPI0_BASE (and associated pins, for PPS parts).

I therefore propose the following changes:

1) Remove all _SPI_BASE defines in all variant files (to avoid confusion) 2) Remove extra pin definitions for non-PPS variants (to avoid confusion), with a comment to that effect (things like _DPSI0_MISO_PIN, etc) 3) Replacing all #if defined(PIC32MX1XX) || defined(PIC32MX2XX) || defined(PIC32MZXX) || defined(PIC32MX47X) with #if defined (PIC32_PPS) for clarity in the SPI library

Now, an added point that Keith brings up is the lack of _SPIx_BASE_ADDRESS defines in the MZ Microchip compiler header files, thus forcing the use of ((uint32_t)&SPI2CON) in place of _SPI2_BASE_ADDRESS in our MZ based variant files. This should be corrected by having somebody on the compiler team add these into the header file, so that the next release of the compiler has them in and we can add the proper defines into our MZ variant files.

If anybody has any issue with any of these changes, please let me know. I'll be submitting a PR with them in the next couple of days for us all to review.

KeithV commented 8 years ago

Brian, Actually I would be in favor of leaving all of the pin definitions even for the non-PPS variants. This is great documentation to know which pins the board designer intended for the SPI, even though this is hard wired for the SPI controller for non-pps MCUs, the documentation in the variant file is wonderful if you are wiring up or scoping your SPI device. In fact, the CS pin is used in some sketches, so please don't remove that one... the others are just good documentation. IMHO

KeithV commented 8 years ago

Oh, also, there is now a #PPS in (I think cpudefs.h) you can use instead of enumerating all of the PPS MCUs in item 3 of your list.

K

From: Brian Schmalz [mailto:notifications@github.com] Sent: Sunday, January 31, 2016 8:43 AM To: chipKIT32/chipKIT-core Cc: KeithV Subject: Re: [chipKIT-core] Remove instances of SPI define in all board variant files (#144)

OK, I've looked into this a bit more, and re-read the various e-mail threads over the past about changes to SPI and such. At this point in time (v1.1.0 of core) we have no libraries or any source code that needs the _SPI_BASE define and the rest of the standard Arduino SPI defines that we have in (most) varient files.

So as far as I can tell, there is no reason for us to have the old SPI defines around anymore. The SPI library that we ship only uses _DSPI0_BASE (and associated pins, for PPS parts).

I therefore propose the following changes:

1) Remove all SPI_BASE defines in all variant files (to avoid confusion) 2) Remove extra pin definitions for non-PPS variants (to avoid confusion), with a comment to that effect (things like DPSI0_MISO_PIN, etc) 3) Replacing all #if defined(__PIC32MX1XX) || defined(PIC32MX2XX) || defined(PIC32MZXX) || defined(PIC32MX47X) with #if defined (PIC32_PPS) for clarity in the SPI library

Now, an added point that Keith brings up is the lack of _SPIx_BASE_ADDRESS defines in the MZ Microchip compiler header files, thus forcing the use of ((uint32_t)&SPI2CON) in place of _SPI2_BASE_ADDRESS in our MZ based variant files. This should be corrected by having somebody on the compiler team add these into the header file, so that the next release of the compiler has them in and we can add the proper defines into our MZ variant files.

If anybody has any issue with any of these changes, please let me know. I'll be submitting a PR with them in the next couple of days for us all to review.

— Reply to this email directly or view it on GitHub https://github.com/chipKIT32/chipKIT-core/issues/144#issuecomment-177546507 .Image removed by sender.

EmbeddedMan commented 8 years ago

Keith,

Fantastic feedback. I think you're absolutely right. I would like to add a comment into the varient files stating that those particular defines (MISO, MOSI) don't actually get used in the SPI constructor for non-PPS parts, just so people don't think that they can change those (in non-PPS parts) and get their pins to change in their sketch.

On Sun, Jan 31, 2016 at 10:54 AM, KeithV notifications@github.com wrote:

Brian, Actually I would be in favor of leaving all of the pin definitions even for the non-PPS variants. This is great documentation to know which pins the board designer intended for the SPI, even though this is hard wired for the SPI controller for non-pps MCUs, the documentation in the variant file is wonderful if you are wiring up or scoping your SPI device. In fact, the CS pin is used in some sketches, so please don't remove that one... the others are just good documentation. IMHO

— Reply to this email directly or view it on GitHub https://github.com/chipKIT32/chipKIT-core/issues/144#issuecomment-177549100 .

EmbeddedMan commented 8 years ago

Keith - yup, I added PIC32_PPS at least year's MASTERs in order to make it more clear what the intent of the #if/#endif is.

*Brian

On Sun, Jan 31, 2016 at 11:00 AM, KeithV notifications@github.com wrote:

Oh, also, there is now a #PPS in (I think cpudefs.h) you can use instead of enumerating all of the PPS MCUs in item 3 of your list.

K

From: Brian Schmalz [mailto:notifications@github.com] Sent: Sunday, January 31, 2016 8:43 AM To: chipKIT32/chipKIT-core Cc: KeithV Subject: Re: [chipKIT-core] Remove instances of SPI define in all board variant files (#144)

OK, I've looked into this a bit more, and re-read the various e-mail threads over the past about changes to SPI and such. At this point in time (v1.1.0 of core) we have no libraries or any source code that needs the _SPI_BASE define and the rest of the standard Arduino SPI defines that we have in (most) varient files.

So as far as I can tell, there is no reason for us to have the old SPI defines around anymore. The SPI library that we ship only uses _DSPI0_BASE (and associated pins, for PPS parts).

I therefore propose the following changes:

1) Remove all SPI_BASE defines in all variant files (to avoid confusion) 2) Remove extra pin definitions for non-PPS variants (to avoid confusion), with a comment to that effect (things like DPSI0_MISO_PIN, etc) 3) Replacing all #if defined(__PIC32MX1XX) || defined(PIC32MX2XX) || defined(PIC32MZXX) || defined(PIC32MX47X) with #if defined (PIC32_PPS) for clarity in the SPI library

Now, an added point that Keith brings up is the lack of _SPIx_BASE_ADDRESS defines in the MZ Microchip compiler header files, thus forcing the use of ((uint32_t)&SPI2CON) in place of _SPI2_BASE_ADDRESS in our MZ based variant files. This should be corrected by having somebody on the compiler team add these into the header file, so that the next release of the compiler has them in and we can add the proper defines into our MZ variant files.

If anybody has any issue with any of these changes, please let me know. I'll be submitting a PR with them in the next couple of days for us all to review.

— Reply to this email directly or view it on GitHub < https://github.com/chipKIT32/chipKIT-core/issues/144#issuecomment-177546507> .Image removed by sender.

— Reply to this email directly or view it on GitHub https://github.com/chipKIT32/chipKIT-core/issues/144#issuecomment-177550964 .

KeithV commented 8 years ago

Correct comments are always good! Good plan, I like it!

From: Brian Schmalz [mailto:notifications@github.com] Sent: Sunday, January 31, 2016 9:00 AM To: chipKIT32/chipKIT-core Cc: KeithV Subject: Re: [chipKIT-core] Remove instances of SPI define in all board variant files (#144)

Keith,

Fantastic feedback. I think you're absolutely right. I would like to add a comment into the varient files stating that those particular defines (MISO, MOSI) don't actually get used in the SPI constructor for non-PPS parts, just so people don't think that they can change those (in non-PPS parts) and get their pins to change in their sketch.

On Sun, Jan 31, 2016 at 10:54 AM, KeithV notifications@github.com wrote:

Brian, Actually I would be in favor of leaving all of the pin definitions even for the non-PPS variants. This is great documentation to know which pins the board designer intended for the SPI, even though this is hard wired for the SPI controller for non-pps MCUs, the documentation in the variant file is wonderful if you are wiring up or scoping your SPI device. In fact, the CS pin is used in some sketches, so please don't remove that one... the others are just good documentation. IMHO

— Reply to this email directly or view it on GitHub https://github.com/chipKIT32/chipKIT-core/issues/144#issuecomment-177549100 .

— Reply to this email directly or view it on GitHub https://github.com/chipKIT32/chipKIT-core/issues/144#issuecomment-177550975 .Image removed by sender.