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

Max6675 SPI conflicts #1227

Closed
Grogyan opened this issue Dec 22, 2014 · 60 comments
Closed

Max6675 SPI conflicts #1227

Grogyan opened this issue Dec 22, 2014 · 60 comments

Comments

@Grogyan
Copy link
Contributor

Grogyan commented Dec 22, 2014

The code for the Max6675 to communicate over the SPI bus assumes that no other devices are on the same bus, and so there is a conflict of communication.
A sanity check to ensure that the SPI bus is free before establishing communication is needed.
Max6675 SS is on pin 66
RAMPS 1.4
RepRap discount full graphic display

@boelle
Copy link
Contributor

boelle commented Dec 26, 2014

related to #1226

@boelle boelle added this to the Bug Fixing Round 3 milestone Jan 6, 2015
@boelle boelle mentioned this issue Jan 9, 2015
@boelle
Copy link
Contributor

boelle commented Jan 13, 2015

one way to solve this one is to use the chip select pin.... if master do not use SPI it will not pull the chip select lines high... one line is needed for each device...

when master wants a temp reading it will pull high the chip select line a and talk to the thermocouple... when done it puts the line low...

http://en.wikipedia.org/wiki/Serial_Peripheral_Interface-bus

@Grogyan
Copy link
Contributor Author

Grogyan commented Jan 14, 2015

https://www.sparkfun.com/datasheets/IC/MAX6675.pdf
MAX6675
http://datasheets.maximintegrated.com/en/ds/MAX31855.pdf
MAX31855

Both require CS to be pulled low (which they are) in order to communicate to

@Grogyan
Copy link
Contributor Author

Grogyan commented Jan 19, 2015

Just pasting this here from an earlier discussion we had on IRC
The MAX6675 uses a bang bang method to simulate basic SPI comms, and is not compatible at all with the SPI comms used in the current implementation of the SD card.

I also took a look at the Repetier code regarding this, they also do Bang Bang for MAX6675 and MAX31855 SPI emulation on the hardware SPI port.

@thinkyhead
Copy link
Member

More pasting on the subject (from this forum) to back up the issue…

The datasheet for the MAX6675 says that it implements a "Simple SPI-Compatible Serial
Interface", which to me suggests that it's a partial implementation of SPI but not a
full one. For example, it doesn't use MOSI at all, only MISO, so communications with
the MAX6675 are read-only. Basically, you enable the chip using the CS (Chip Select)
line and it just starts spitting data at you. It *looks* like the intention is that it
can share a bus with real SPI devices, so I assume that if you de-assert CS it'll shut
up and leave the bus for other devices to communicate.

Speculation there on possible solutions, but no resolution.

@thinkyhead
Copy link
Member

We have a suggestion from David Braam to move the MAX6675 code into the main thread (updateTemperaturesFromRawValues) using this commit from his Ultimaker fork.

Ultimaker/Ultimaker2Marlin@91c64e3

@thinkyhead thinkyhead self-assigned this Jan 23, 2015
thinkyhead added a commit to thinkyhead/Marlin that referenced this issue Jan 23, 2015
Changes to temperature.cpp from Ultimaker fork, intended to address
MarlinFirmware#1226 and MarlinFirmware#1227
thinkyhead added a commit to thinkyhead/Marlin that referenced this issue Jan 24, 2015
Changes to temperature.cpp from Ultimaker fork, intended to address
MarlinFirmware#1226 and MarlinFirmware#1227
@thinkyhead
Copy link
Member

#1402 should have resolved this. We don't have a more comprehensive solution at this time, AFAIK. The best authority on this issue is probably @daid .

@boelle
Copy link
Contributor

boelle commented Mar 6, 2015

on phone but i remember that spi stuff needs one digital line for each
device besides the data line... mcu pulls the line high when it wants to
talk to a device

Den fredag den 6. marts 2015 skrev Scott Lahteine <notifications@github.com

:

#1402 #1402 should have
resolved this. We don't have a more comprehensive solution at this time,
AFAIK. The best authority on this issue is probably @daid
https://github.com/daid .


Reply to this email directly or view it on GitHub
#1227 (comment)
.

@boelle
Copy link
Contributor

boelle commented Mar 6, 2015

with one device it does not matter but with 2 or more it does... so even
with one we should do it

Den fredag den 6. marts 2015 skrev Bo Herrmannsen <bo.herrmannsen@gmail.com

:

on phone but i remember that spi stuff needs one digital line for each
device besides the data line... mcu pulls the line high when it wants to
talk to a device

Den fredag den 6. marts 2015 skrev Scott Lahteine <
notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');>:

#1402 #1402 should have
resolved this. We don't have a more comprehensive solution at this time,
AFAIK. The best authority on this issue is probably @daid
https://github.com/daid .


Reply to this email directly or view it on GitHub
#1227 (comment)
.

@daid
Copy link
Contributor

daid commented Mar 6, 2015

Actually https://github.com/marwijn contributed the patch to me. I know little more then that.

@thinkyhead
Copy link
Member

Eek! Well, I have heard some interesting things about the new SPI libraries that come with Arduino 1.6. Worth looking into, perhaps.

@boelle
Copy link
Contributor

boelle commented Mar 7, 2015

http://upload.wikimedia.org/wikipedia/commons/f/fc/SPI_three_slaves.svg

SPI can be daisy chained but then a shift register is needed and i very
much doubt that any 3d printer stuff that uses SPI will have that
implemented...

So our first Q to ourselves is: Does spi stuff use a slave select line for
each device?

if not we need to implement that so that we know what device we are doings
comms with... or else we will get conflicts... an SPI display and a SPI
thermocouple is an example... they will clash pretty fast :-D

2015-03-07 0:11 GMT+01:00 Scott Lahteine notifications@github.com:

Eek! Well, I have heard some interesting things about the new SPI
libraries that come with Arduino 1.6. Worth looking into, perhaps.


Reply to this email directly or view it on GitHub
#1227 (comment)
.

@scotty1024
Copy link

http://www.maximintegrated.com/en/app-notes/index.mvp/id/3947

On Mar 7, 2015, at 2:20 AM, Bo Herrmannsen notifications@github.com wrote:

http://upload.wikimedia.org/wikipedia/commons/f/fc/SPI_three_slaves.svg

SPI can be daisy chained but then a shift register is needed and i very
much doubt that any 3d printer stuff that uses SPI will have that
implemented...

So our first Q to ourselves is: Does spi stuff use a slave select line for
each device?

if not we need to implement that so that we know what device we are doings
comms with... or else we will get conflicts... an SPI display and a SPI
thermocouple is an example... they will clash pretty fast :-D

2015-03-07 0:11 GMT+01:00 Scott Lahteine notifications@github.com:

Eek! Well, I have heard some interesting things about the new SPI
libraries that come with Arduino 1.6. Worth looking into, perhaps.


Reply to this email directly or view it on GitHub
#1227 (comment)
.


Reply to this email directly or view it on GitHub.

@boelle
Copy link
Contributor

boelle commented Mar 7, 2015

and the purpose of the link? nothing new info in there that i have not
allready told

2015-03-07 18:38 GMT+01:00 scotty1024 notifications@github.com:

http://www.maximintegrated.com/en/app-notes/index.mvp/id/3947

On Mar 7, 2015, at 2:20 AM, Bo Herrmannsen notifications@github.com
wrote:

http://upload.wikimedia.org/wikipedia/commons/f/fc/SPI_three_slaves.svg

SPI can be daisy chained but then a shift register is needed and i very
much doubt that any 3d printer stuff that uses SPI will have that
implemented...

So our first Q to ourselves is: Does spi stuff use a slave select line
for
each device?

if not we need to implement that so that we know what device we are
doings
comms with... or else we will get conflicts... an SPI display and a SPI
thermocouple is an example... they will clash pretty fast :-D

2015-03-07 0:11 GMT+01:00 Scott Lahteine notifications@github.com:

Eek! Well, I have heard some interesting things about the new SPI
libraries that come with Arduino 1.6. Worth looking into, perhaps.


Reply to this email directly or view it on GitHub
<
#1227 (comment)

.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
#1227 (comment)
.

@Grogyan
Copy link
Contributor Author

Grogyan commented Mar 7, 2015

There is another device that uses the 1 wire protocol, which can easily be
used to daisy chain multiple thermocouples.

Alas I don't have one, nor have I got time to test the current development
branch to see if the fix that was put in about a month ago has solved the
SPI issue.
On 8 Mar 2015 07:02, "Bo Herrmannsen" notifications@github.com wrote:

and the purpose of the link? nothing new info in there that i have not
allready told

2015-03-07 18:38 GMT+01:00 scotty1024 notifications@github.com:

http://www.maximintegrated.com/en/app-notes/index.mvp/id/3947

On Mar 7, 2015, at 2:20 AM, Bo Herrmannsen notifications@github.com
wrote:

http://upload.wikimedia.org/wikipedia/commons/f/fc/SPI_three_slaves.svg

SPI can be daisy chained but then a shift register is needed and i very
much doubt that any 3d printer stuff that uses SPI will have that
implemented...

So our first Q to ourselves is: Does spi stuff use a slave select line
for
each device?

if not we need to implement that so that we know what device we are
doings
comms with... or else we will get conflicts... an SPI display and a SPI
thermocouple is an example... they will clash pretty fast :-D

2015-03-07 0:11 GMT+01:00 Scott Lahteine notifications@github.com:

Eek! Well, I have heard some interesting things about the new SPI
libraries that come with Arduino 1.6. Worth looking into, perhaps.


Reply to this email directly or view it on GitHub
<

#1227 (comment)

.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
<
#1227 (comment)

.


Reply to this email directly or view it on GitHub
#1227 (comment)
.

@scotty1024
Copy link

I think that document's diagrams more clearly demonstrate that so long as you chain SPI devices properly there is no conflict: AVR -> Thermocouple -> Viki LCD.

On Mar 7, 2015, at 10:02 AM, Bo Herrmannsen notifications@github.com wrote:

and the purpose of the link? nothing new info in there that i have not
allready told

2015-03-07 18:38 GMT+01:00 scotty1024 notifications@github.com:

http://www.maximintegrated.com/en/app-notes/index.mvp/id/3947

On Mar 7, 2015, at 2:20 AM, Bo Herrmannsen notifications@github.com
wrote:

http://upload.wikimedia.org/wikipedia/commons/f/fc/SPI_three_slaves.svg

SPI can be daisy chained but then a shift register is needed and i very
much doubt that any 3d printer stuff that uses SPI will have that
implemented...

So our first Q to ourselves is: Does spi stuff use a slave select line
for
each device?

if not we need to implement that so that we know what device we are
doings
comms with... or else we will get conflicts... an SPI display and a SPI
thermocouple is an example... they will clash pretty fast :-D

2015-03-07 0:11 GMT+01:00 Scott Lahteine notifications@github.com:

Eek! Well, I have heard some interesting things about the new SPI
libraries that come with Arduino 1.6. Worth looking into, perhaps.


Reply to this email directly or view it on GitHub
<
#1227 (comment)

.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
#1227 (comment)
.


Reply to this email directly or view it on GitHub.

@boelle
Copy link
Contributor

boelle commented Mar 7, 2015

yep... might get one of those thermocouple boards (got the thermocouple) so
that we can test... are those full graph displays for ramps also spi?

2015-03-07 20:00 GMT+01:00 scotty1024 notifications@github.com:

I think that document's diagrams more clearly demonstrate that so long as
you chain SPI devices properly there is no conflict: AVR -> Thermocouple ->
Viki LCD.

On Mar 7, 2015, at 10:02 AM, Bo Herrmannsen notifications@github.com
wrote:

and the purpose of the link? nothing new info in there that i have not
allready told

2015-03-07 18:38 GMT+01:00 scotty1024 notifications@github.com:

http://www.maximintegrated.com/en/app-notes/index.mvp/id/3947

On Mar 7, 2015, at 2:20 AM, Bo Herrmannsen <notifications@github.com

wrote:

http://upload.wikimedia.org/wikipedia/commons/f/fc/SPI_three_slaves.svg

SPI can be daisy chained but then a shift register is needed and i
very
much doubt that any 3d printer stuff that uses SPI will have that
implemented...

So our first Q to ourselves is: Does spi stuff use a slave select
line
for
each device?

if not we need to implement that so that we know what device we are
doings
comms with... or else we will get conflicts... an SPI display and a
SPI
thermocouple is an example... they will clash pretty fast :-D

2015-03-07 0:11 GMT+01:00 Scott Lahteine notifications@github.com:

Eek! Well, I have heard some interesting things about the new SPI
libraries that come with Arduino 1.6. Worth looking into, perhaps.


Reply to this email directly or view it on GitHub
<

#1227 (comment)

.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
<
#1227 (comment)

.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
#1227 (comment)
.

@Grogyan
Copy link
Contributor Author

Grogyan commented Mar 8, 2015

I don't think the MAX31855 board is supported yet in Marlin.
Which thermocouple board(s) will you be getting?

@boelle
Copy link
Contributor

boelle commented Mar 8, 2015

one that is know to cause issues so we can get it solved...

2015-03-08 22:33 GMT+01:00 Grogyan notifications@github.com:

I don't think the MAX31855 board is supported yet in Marlin.
Which thermocouple board(s) will you be getting?


Reply to this email directly or view it on GitHub
#1227 (comment)
.

@thinkyhead
Copy link
Member

@Grogyan I'd like to see your configuration files, specifically looking for any complex defines that might have calculations in them, but which don't have parentheses around them, just in case there's some code like d = PID_MAX / 2; being expanded, when it should be d = (PID_MAX) / 2; to get the right value. Meanwhile, still comparing the temperature code from 1.0.2 to the current, but nothing has jumped out yet.

@AnHardt
Copy link
Member

AnHardt commented Mar 22, 2015

Got my MAX6675 this morning but will not be able to connect before Monday.

In the meanwhile one could set a jumper on the open (i suppose) analog temperature pins.
When the behavior changes (MIN<>MAX) we know that we are somewhere asking the analog ports instead of the MAX6675.

@Grogyan
Copy link
Contributor Author

Grogyan commented Mar 22, 2015

Configuration.h
http://pastebin.com/MWE81C1V
Configuration_adv.h
http://pastebin.com/DbDQxWsd

Nothing special being done to the config file.
Just setting the thermocouple, and enable the Full Graphic display

@thinkyhead
Copy link
Member

Ok, with the MAX6657 this code snippet is questionable…

#ifdef HEATER_0_USES_MAX6675
  ...
  OUT_WRITE(MAX6675_SS,HIGH);

  // The line above replaces this from 1.0.2:
  //pinMode(MAX6675_SS, OUTPUT);
  //digitalWrite(MAX6675_SS,1);

#endif //HEATER_0_USES_MAX6675

Which translates into:

DIOMAX6675_SS |= MASK(DIOMAX6675_SS_PIN);
if (&(DIOMAX6675_SS_RPORT) >= (uint8_t *)0x100) {
  _WRITE_C(MAX6675_SS, 1);
}
else {
  _WRITE_NC(MAX6675_SS, 1);
}

So, perhaps that would be good to revert...? Or was 1.0.2 incorrect?

@Grogyan
Copy link
Contributor Author

Grogyan commented Mar 23, 2015

I don't believe the MINTEMP error is actually referring to an error with the Temperature code.
But rather, maybe something not getting initialized properly at boot.

This is the output log of Repetier, note that I also get the error even with a thermistor
http://pastebin.com/EWVAw7Se

Something getting lost with stuff loaded into EEPROM, even when EEPROM is not enabled?
I'm just banging rocks here. Just really eager to test out the fix for the MAX6675.

In my RAMPS setup, I did load factory settings into EEPROM a month or two ago, since then I have not enabled it again till this error is resolved.

@AnHardt
Copy link
Member

AnHardt commented Mar 23, 2015

Partially good news for the SPI-SD-MAX6675-complex.

Connected my new MAX6675 this morning and got good temperature readings but the MINTEMP-error. (Arduino 1.6.1 - current Marlin including merge #1675 - FULL_GRAPHICS_CONTROLLER)
Commented out 'temperature.cpp' lines 541 and 542 to get rid of the MINTEMP-errors by brute force.
Uploaded a big file to the SD-Card via Pronterface.
Compared the new file with the same previously uploaded file without the MAX6675.
No corruption!
Got some differences where M105-statements are distributed differently. This seems to be a problem with Pronterface what does not stop to ask for temperatures during upload.

I'm confident that the SPI-SD-MAX6675 problem is solved when we have caught the MINTEMP bug.

Going back to MINTEMP problem now.

@AnHardt
Copy link
Member

AnHardt commented Mar 23, 2015

Until after #1527 (before #1541) i can produce working code for MAX 6675 by forward declaration of : void max_temp_error(uint8_t e);
void bed_max_temp_error(void);
void min_temp_error(uint8_t e);
in 'temperature.cpp' line around line 197.

@AnHardt
Copy link
Member

AnHardt commented Mar 23, 2015

Her i come closer to the reason why MAX6675 gives a MINTEMP error.
Until SD-Card is ether initialized (echo:SD card ok) or had failed (echo:SD init fail) readout is 0.

...
0 actual
1087 max
32 min
!
0
1087
32
!
echo:SD init fail
89
1087
32
!
89
1087
32
!
...

...
0
1087
32
!
0
1087
32
!
echo:SD card ok
91
1087
32
!
91
1087
32
!
...

@Grogyan
Copy link
Contributor Author

Grogyan commented Mar 24, 2015

Coooool bananas.
Probably won't be able to test out this fix tonight

@Grogyan
Copy link
Contributor Author

Grogyan commented Mar 24, 2015

Tried out the latest dev branch again, I still get the MINTEMP error, but then again I cannot see the actual changes very well, as there has been a lot of changes done from multiple issues around the temperature.cpp
Comparing with what I just downloaded and comparing it with the git changelog.

@thinkyhead
Copy link
Member

@Grogyan The most recent fix around temperature got rid of the test of the thermistor in cases where a MAX6675 is in use, and made sure the redundant temp sensor option had storage for the second thermistor.

@thinkyhead
Copy link
Member

At the moment Marlin seems to be okay with simple thermocouple setups. This page is getting long and unwieldy, so let's start a new issue if there's more information to discuss, linking back to this one for all the useful links.

@Grogyan
Copy link
Contributor Author

Grogyan commented Mar 31, 2015

I only just uploaded the new firmware as of 2 days ago.
Looks fixed, but cannot yet verify, damn RL.
I get good temp readings and no MINTEMP errors.

On Wed, Apr 1, 2015 at 1:41 AM, Scott Lahteine notifications@github.com
wrote:

Closed #1227 #1227.


Reply to this email directly or view it on GitHub
#1227 (comment).

@khessels
Copy link

khessels commented Feb 8, 2018

I know its closed, but for history sake in case someone finds himself with the same issue.
I observed the same problem with the mintemp and the max6675. It seemed to function well, except for when an axis started moving and it would show the mintemp error.

I had extended the sensor with regular wire, however it picked up a lot of (motor?) noise that got into the mksgen1.4. Moving the sensor closer to the printhead\sensor and extending the spi solved it.

@thinkyhead
Copy link
Member

thinkyhead commented Feb 14, 2018

Thanks for the tip! It has also been long recommended to use NC switches for endstops and avoid entangling them with stepper wires to avoid false endstop triggers due to induction.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants