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

working code for sigma delta generator #4119

Closed
wants to merge 17 commits into from
Closed

working code for sigma delta generator #4119

wants to merge 17 commits into from

Conversation

stefaandesmet2003
Copy link

provide a simple interface to attach/detach pins to the sigma-delta generator, and get/set the 2 parameters prescaler & target

provide a simple interface to attach/detach pins to the sigma-delta generator, and get/set the 2 parameters prescaler & target
@devyte
Copy link
Collaborator

devyte commented Jan 9, 2018

@stefaandesmet2003 this is for #4119 , yes? I see you figured it out :)

@stefaandesmet2003
Copy link
Author

Yes, indeed

@devyte devyte added this to the 2.6.0 milestone Jan 10, 2018
The sigma delta source remains on until :
6. sigma_delta_disable()

copy this code as example :
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example should be made into a working example that builds with the rest of the core, so that at least build issues can be tracked.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. The example would then be under libraries\esp8266\examples ?

THE sigma delta output frequency is defined by the ESP8266 hardware as:

FREQ = 80,000,000/prescaler * target /256 HZ, 0<target<128
FREQ = 80,000,000/prescaler * (256-target) /256 HZ, 128<target<256
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these constant even in 2x mode where CPU_F = 160,000,000?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, i never tested 160MHz. Will verify and correct accordingly!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked : CPU frequency has no influence on the SD frequencies

For higher value capacitors, it might be necessary to add a high-speed buffer between the esp8266 output and RC-filter to limit the current ac load on the
pin output driver.

*/
Copy link
Collaborator

@earlephilhower earlephilhower Jan 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add attribution and license (CC0 or PD) to the example header?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add attribution and license (CC0 or PD) to the example header?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PD i guess?

Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall good stuff here! Some minor suggestions and inquiries to make sure it's easy for Arduino users who might not be up on the internals.

void sigma_delta_setTarget(uint8_t target);
uint8_t sigma_delta_getPrescaler(void);
void sigma_delta_setPrescaler(uint8_t prescaler);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good, but I think to be most useful to end users, who may not be as familiar w/ clock dividers and sigma-delta operation a simpler API might be helpful.

I'd recommend renaming "setTarget/GetTarget" to simply, "write" or "read" as effectively they're trying to write an analog value and "target," while the name used by the hardware designers in the HDK, doesn't really mean much if you don't understand the error-diffusion behind things.

I'd also suggest a simpler "setFrequency(long f) setter method that would do the math described in the header and figure out the closest prescaler given the F_CPU. That'd make end-user code simpler to understand.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I fully understand the setFrequency idea. Every call to 'write' (setTarget) would need to adapt the prescaler as well to fit this chosen frequency? And remove the setPrescaler/getPrescaler functions altogether?
There was some sample code like this in an earlier post to this issue. In many cases the effective output frequency is way off the chosen frequency f. Suppose you chose 1 MHz. With 50% duty cycle the closest you get is 312.5kHz. So you give the user the impression that the generator can be used as a fixed frequency source, but in fact it cannot.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I withdraw this silly request. :) I was thinking this would be useful for someone to, say, drive a 4-pin PWM fan where they'd need to set frequency = 25khz, duty = %age, but it seems it's not really precise enough for such a use, anyway.

For the examples, please do add the Public Domain attribution. I think the team is trying to get these examples cleaned up and that's an easy thing to do.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently ESP32 has an implementation similar to what you suggested. I modified the code to be compatible. ESP32 has apparently 8 independent channels, esp8266 has only 1. Apart from that, the behavior is the same (eg. in terms of output frequencies.) So it makes sense to align the programming interfaces.
Arduino-esp32 also provides a working sigma delta example; there it's not necessary to explicitly include the sigma_delta.h file in the example code; it would be convenient if this was the same in arduino-esp8266, but my example currently doesn't work without the explicit include

For higher value capacitors, it might be necessary to add a high-speed buffer between the esp8266 output and RC-filter to limit the current ac load on the
pin output driver.

*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add attribution and license (CC0 or PD) to the example header?

@earlephilhower earlephilhower modified the milestones: 2.6.0, 2.5.0 Mar 9, 2018
Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that 2.4.1 is out this can be queued for the 2.5.0 merge. Only a couple minor things. Thanks!

* Parameters : none
* Returns : uint8 prescaler, CLK_DIV , 0-255
* FunctionName : sigmaDeltaRead
* Description : set the duty cycle for the sigma-delta source
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo here...this "get the duty..."

@@ -0,0 +1,152 @@
#include "Arduino.h" // using pinMode
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs standard licence header, please.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok done

}

/******************************************************************************
* FunctionName : sigma_delta_attachPin
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some FunctionName comments got updated to camelCase, but some like this did not.

#ifndef SIGMA_DELTA_H
#define SIGMA_DELTA_H

#ifdef __cplusplus
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function prototypes here are "extern "C""'d, but the actual function definitions aren't. Any reason not to drop the "extern C" here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linking stage doesn't work without this. Maybe you know why?
/SigmaDeltaDemo.ino:16: undefined reference to `sigmaDeltaGetPrescaler()'
and the same for all other sigmaDelta functions

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore my previous comment. The other "core_esp8266_*" files are all .c. So ignore the request to remove extern C, leave it there. Sorry for the confusion!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @earlephilhower ,
I don't manage to get the style_check right on the travis build. Could you have a look what the issue is, please?
The other builds are ok now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do tonight. I think it's just a matter of getting the examples in Arduino IDE format (i.e. 2 space indent, brackets in the right spot, etc.).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like your problem is DOS linefeeds. Can you save it with Unix ones, that seems to be what IGRR's checker requires to work properly. I can't do it for you as I don't have write permission to your source repo (nor should I, of course!).

@earlephilhower
Copy link
Collaborator

I can't upload to your repo, but I can give you the file to upload yourself. Rename to .ino and replace your existing sample and I think you'll be all set:

SigmaDeltaDemo.ino.txt

@stefaandesmet2003
Copy link
Author

thanks for your help.
after 2 more attempts it still fails.
I quit,sorry.

@earlephilhower
Copy link
Collaborator

@stefaandesmet2003 Sorry for the run around! The Windows/Unix line ending difference is just breaking things for you.

I'll try and pull your branch into my Arduino fork, fix it, and open a linked bug just to get this done.

@earlephilhower
Copy link
Collaborator

Just committed your changes, closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants