bengtmartensson / AGirs

A Girs infrared server for the Arduino
http://www.harctoolbox.org/Girs.html
GNU General Public License v2.0
55 stars 13 forks source link

Implementet IRSENSOR_X_PULLUP X = 1,2,... #15

Closed bengtmartensson closed 8 years ago

bengtmartensson commented 8 years ago

Applies to both AGirs and GrisLite, (possibly Girs4Lirc). Cf https://github.com/bengtmartensson/harctoolboxbundle/issues/92#issuecomment-174245232

For this, there should be a second argument of IrWidgetAggregating::newIrWidgetAggregating in capture(Stream&).

BartmanEH commented 8 years ago

As a work-around, can I simply add the following to GirsUtils.h:

#ifdef IRSENSOR_1_PIN
        pinMode(IRSENSOR_1_PIN, INPUT_PULLUP);
#endif

I know this isn't a scalable solution but rather a hack. Will it work? I can't tell if it will conflict with code elsewhere in the project.

bengtmartensson commented 8 years ago

No it will not work. Try the commit instead.

BartmanEH commented 8 years ago
sketch\GirsLite.cpp: In function 'boolean capture(Stream&)':
GirsLite.cpp:293: error: 'sensorPullup' is not a member of 'GirsUtils'
             GirsUtils::sensorPullup(sensorNo));
bengtmartensson commented 8 years ago

There must be something "stale" somewhere, like an old version of GirsUtils.h, likely in the library.

BartmanEH commented 8 years ago

I believe you were correct. I uninstalled the IDE, deleted the AppData folder manually, rebooted windows as suggested here, reinstalled the IDE, recopied the libraries GirsLib, Infrared4Arduino and _LiquidCrystalI2C, and unbelievably I was able to compile it! Testing will be tomorrow evening once I get my Pro Micro and remote controls back into my hands.

BartmanEH commented 8 years ago

This is awesome. I poked around the newly committed source code to learn about the new _IRSENSOR_XPULLUP X = 1,2,... implementation. I want to thank Bengt for his amazing work here - going beyond his own needs and adding a lot of new code to support alternative hardware like mine. Now I can just go into the pin definition file _girs_pinsmicro.h for my Pro Micro or _girs_pinsdefault.h for my Uno R3 Plus and just uncomment the pullup line here:

#if defined(CAPTURE)
// capture pin (ICP) 4
#define IRSENSOR_1_PULLUP

...and have the ATmega chip do the hard work of pulling up my open collector output on my QSE159 IR sensor rather than having to sneak in a resistor in the hardware. Thanks so much!

BartmanEH commented 8 years ago

The latest committed AGirs I think is missing the pre-processor ability to detect and use _girs_pinsmicro.h since the config.h file has:

// Include one file describing the pin configuration
#ifdef ARDUINO_AVR_NANO
//#include <girs_pins_nano_shield.h>
#include <girs_pins_nano.h>
#else
#include <girs_pins.h> // Generic
#endif

which I believe should be:

// Include one file describing the pin configuration
#ifdef ARDUINO_AVR_NANO
//#include <girs_pins_nano_shield.h>
#include <girs_pins_nano.h>
#elif
#ifdef ARDUINO_AVR_MICRO
#include <girs_pins_micro.h>
#else
#include <girs_pins.h> // Generic
#endif

The verbose output of a compile with the Arduino IDE doesn't show the usage of either _girs_pinsdefault.h or any other _girspins[board].h_ file so I'm not sure how to prove this.

BartmanEH commented 8 years ago

Nevermind... I see that in _girspins.h there are further #elif directives:

#if defined(ARDUINO_AVR_NANO)
#include "girs_pins_nano.h"
#elif defined(ARDUINO_AVR_MICRO)
#include "girs_pins_micro.h"
[...]

I can also confirm that the pull up directive now works. I uncommented _#define IRSENSOR_1PULLUP in _girs_pinsdefault.h:

#if defined(CAPTURE)
#define IRSENSOR_1_PIN 8
// capture pin (ICP) 8/4/49
#define IRSENSOR_1_PULLUP
#endif

Now when I use IrScrutinzer to enable Capture mode, magically the IR sensor signal pin attached to pin 8 of my Uno jumps up to 5VDC even though I've removed by physical external pull up resistor :-)

bengtmartensson commented 8 years ago

You have missed the semantics of the line // Include one file describing the pin configuration

It means that the user is supposed to add/adapt a/the include statements for a/the desired configuration file. Of course, if maintaining several configurations, using preprocessor symbols may be handy, if the user so desires. My aim is not to have a ladder of interleaved #ifdef and #includes. So I do not consider it a mistake.

Of course, suggestions for improvements are welcome.

Glad that the pullup works for you, closing.

BartmanEH commented 8 years ago

There is already a ladder of interleaved #ifdef and #includes in _girspins.h - what's confusing is that it is in two different places. Perhaps the ladder from _girspins.h could be moved to the corresponding section in config.h

bengtmartensson commented 8 years ago

Thank you for the suggestion. I like to emphasize "User, add the file YOU want" instead,

The verbose output of a compile with the Arduino IDE doesn't show the usage of either girs_pins_default.h or any other girspins[board].h file so I'm not sure how to prove this.

If I want to check that a certain file is included, i just put

#error blahblah

into it and check for an error `blahblah' when compiling.

BartmanEH commented 8 years ago

#error blahblah is a great tip, thanks for that. I've put it in my personal Cheat Notes file.