Stored the current position on reboot

[AndreiCo14]

If a position other than 0 was previously stored, then when the controller is restarted, the stored value is reduced by 1 and the new position stored in the NVRAM.

[RobTillaart]

Thanks for the issue.
I do not fully understand the issue in terms when it does happen and what triggers it.
The library code does not decrement any stored position when starting.

Only when you call void begin(uint8_t pulsePin, uint8_t directionPin, uint8_t selectPin, uint8_t position = 0)
you must set a position if you want it to be non zero.

Can you post a minimal sketch that shows the problem?

[AndreiCo14]

Apologize. Conducted tests without a library. It looks like the problem is in the chip itself. Writing to NVRAM occurs always when CS=HIGH and does not depend on the state of INC.

[RobTillaart]

If I recall correctly the trigger to store is the rising low -> high of the CS line. Precondition is that the inc line is HIGH, there is a table in the datasheet, you should check it.

That said it is a strange problem as if you restart the chip several times it would drop to zero.

It might be useful to add some notes to the readme.md about this, so keep me informed of your insights.

[AndreiCo14]

That's exactly what happens.
I think the chip is fake and all the problems are because of this.
Changed the sequence in X9C10X.cpp . This helped not to decrease the value every time you reboot.

  digitalWrite(_selectPin,    HIGH);
  digitalWrite(_pulsePin,     HIGH);
  digitalWrite(_directionPin, HIGH);

  pinMode(_selectPin, OUTPUT);
  pinMode(_pulsePin, OUTPUT);
  pinMode(_directionPin, OUTPUT);

[RobTillaart]

Good observation,
Need to verify the datasheet, as this would be a bug that should not happen.

[RobTillaart]

@AndreiCo14
Created a develop branch - version 0.2.0 - to reorder the startup sequence of the IO (and some more).

If you have time could you check / verify the working of the branch

[RobTillaart]

There is a problem with the build-CI.
I am investigating .

[RobTillaart]

build-CI is running again.
It cannot do while(micros() < 500); right as micros() keep on returning 0.
Known problem in CI - Arduino-CI/arduino_ci#241

[RobTillaart]

(for ref purpose)

[RobTillaart]

@AndreiCo14
No experience with the store() functionality, so a question you might be able to answer.

In begin() I have the parameter position = 0.
The value set (or default) is naively copied to the internal variable _position.

Q1: Should it not be better to use setPosition(position, true) as last line, to be sure the device and library are in sync?
As far as I know there exist no way to read the state of the device.
So if the restarts from a value from NV-RAM it is impossible to get the internal variable in sync.
Thinking about the restart I would always call **setPosition(pos, true) just after begin() so I would know the position.

Q2: Or should the position parameter be removed from begin()?

---

Alternative thought is to have a library that does not know anything about the position, and is just able to send INCR and DECR and STORE commands. That would be very minimalistic and still usable. Feedback about the position should be organized some other way.

[RobTillaart]

Maybe even better, the 3 lines should use pull-up resistors, so they are always HIGH at start.
Would also give nicer square pulses.

Could add this as advice in the readme.md

[AndreiCo14]

In begin() I have the parameter position = 0.

I think that in order not to mislead by indicating the position in begin, which does not affect anything, it is better to remove this parameter. The user himself can set the position forcibly.

[AndreiCo14]

Maybe even better, the 3 lines should use pull-up resistors, so they are always HIGH at start.

My first experience showed that when restarting the controller, random position values were set. After that, I pulled CS to power. After that, I began to start adequately, but with the problem described above. ;)

[RobTillaart]

Updated the library

• removed the implicit position parameter from begin()
• updated the readme
  • explicit call to setPosition() needed
  • added an few lines on pull up resistors
• updated the examples

Like to hear your opinion if this is an improvement. IMHO making the position more explicit is a good step.
However it is in conceptual conflict with not knowing the stored value in the device.
The store() function seems to add little value if one does not know it after a restart.

Need time to build a setup to do real HW testing, to get a better feel of the merits of this conflict.

[AndreiCo14]

However it is in conceptual conflict with not knowing the stored value in the device.

The user can decide for himself whether to set explicit values or relative to the stored one. In addition, it is not always necessary to know the numeric value (for example, to save the sound level).

[RobTillaart]

Understand your point, however after a restart one does not know at which level the volume was stored and therefor at which volume level it will start. That makes it impossible to set the (internal) position correctly, or by all means to drop to 1/3rd of current volume as one does not know how many steps are needed. (note if the user does it interactively by listening and adjust there is a feedback loop outside the library)

In the current code e.g. the incr() function takes the current position into account.
If it is at 99 (the max) it stops incrementing the value. But it can only know if it is at the max if the start position is set correct.

This conflict leads to 2 different uses,

• a library that does not track the position, but just does increment and decrement.
  One cannot ask for the position as that is not known. The device (position) is in control so to say.
  This model will work but has problems to give adequate feedback about position unless it is set explicitly by the user.
• a library that does track the position but which can only works correctly if the stored value in the device is overruled at some moment (right after the start preferably ). Being able to give position feedback adds valuable functionality in a project.

It is possible to add an ADC to provide the position feedback in another way but that is out of the scope of this library.

The best solution I can think of at the moment is

• a base class that does not keep track of position and allows store() incr() and decr()
• a derived class that adds explicit position information to that. This will look more like the current lib.

This hierarchy would allow users to choose which functionality they need in their project.

[RobTillaart]

Extracted a X9C base class which is minimalistic, no position or Ohm information.
This class gives the user the option to handle the position himself.

This develop branch version will be merged coming days to keep the delta between versions not too large.
(unless there are bugs in current code)

[RobTillaart]

@AndreiCo14
Any new insights or remarks?
If not I will going to merge into master tomorrow.