Wrong clock divider from setFrequency

[UlliBien]

Wrong clock divider from setFrequency(frq) if frq is below the minimal frequency (152 at 80 MHz processor clock). setFrequency passes 0x7FFFF000 to setClockDivider. For the correct setting of regL this should be 0x7FFFF020. 0x7FFFF020 is the value you get when you call setFrequency(152).

[TD-er]

What frequency are we talking about here?
A quick look only shows one to set the frequency of the SPI.
Can you give a link to the right file/linenr here?

[UlliBien]

sorry, I'm not very familar with github. I did not mention that this column is valid for the complete esp8266/Arduino.

Yes, I'm speaking about SPI.

In spi.cpp in method SPIClass::setFrequency in line 215 the value of minFreqReg is set to 0x7FFFF000:
const spiClk_t minFreqReg = { 0x7FFFF000 };

In line 219 this value ist passed to method setClockDivider:
setClockDivider(minFreqReg.regValue);

setClockDivider sets this value without modification to the SPI clock register (line 290):
SPI1CLK = clockDiv;

However, the twelve least significant bits of this register are responsible for the duty cycle. 0x0 is not correct here. The schould be filled with 0x020 for 0x7FFFF in the higher Bits.

The best solution is replacing 0x7FFFF000 by 0x7FFFF020 in line 215

[TD-er]

Just for my understanding.
What effects would we see with the incorrect duty cycle?
In other words, what is being fixed when this is changed?

It looks like this line was changed 4 years ago in this commit: d7a88c3

[UlliBien]

Here's a good explanation for the SPI CLOCK register: www.esp8266.com. Look at "SPI_CLKCNT_H & SPI_CLKCNT_L". If the difference of both parts is zero no clock signal is generated. The difference schould be something about half of the number of SPI clock cycles.

The problem only occurs when a frequency value is specified that is smaller than the minimum of 152 Hz. In this case 0x7FFFF000 is taken with zero values for both SPI_CLKCNT_H and SPI_CLKCNT_L . 0x7FFFF020 is the correct value. setFrequency calculates this value for 152 Hz but gives 0x7FFFF000 for 151 an lower.

I assume that this error is rarely a problem, because such a low clock frequency is rarely used.

[earlephilhower]

Oops, I read this as I2C and not SPI and was going to link it to the rewrite.

@UlliBien, this would be a very simple PR for you. Feel like taking it up and getting stuck in the git blame?

[UlliBien]

Sorry, as mentioned I'm not familiar with github. I don't know how to do that.

[thangktran]

i'll be working on this issue.

[UlliBien]

As mentioned I'm not familiar with github. Looking at "spi.cpp" there ist still the wrong statement. when does the change take effect?

[devyte]

@UlliBien a PR (=pull request) was made by @thangktran as #6409. You can check his proposed code change in thst link. The PR is not merged yet, which is why you don't see it in the code base.
If you agree with the proposed code change, and test it successfully, I'll merge it (rule of thumb: one user proposes a PR, another user confirms that it fixes the issue => usually enough to merge).
There are a lot of tutorials out there for testing a PR, it's rather easy once you install latest git. If after trying you still need help, maybe @thangktran can guide you, or someone else.
Alternatively, you can manually edit your local core source file with the proposed change, given that in this case it's just a 1-liner. I very strongly advise against making a habit of that, though.